Am 10.08.23 um 02:41 schrieb Jeff King:
> On Wed, Aug 09, 2023 at 06:41:13PM +0200, René Scharfe wrote:
>
>> And sorry for the unused parameter warning. Just checked; there are
>> 170+ of those remaining before we can enable it in developer mode. :-/
>> Seems worthwhile, though, especially not slapping UNUSED blindly on
>> them.
>
> I know, I'm working on it. :) There were more than 800 when I started.
> I'm hoping to send out the final patches during this 2.43 cycle.
>
>> Oh. Do we really need all those? Anyway, if we added at least the
>> most common ones, we'd be better off already, I'd expect:
>>
>> $ % git grep -h ' = opt->value;' | sed 's/\*.*$//; s/^ *//' | sort | uniq -c | sort -nr | head -10
>> 29 struct diff_options
>> 12 int
>> 7 struct grep_opt
>> 6 struct rebase_options
>> 6 struct apply_state
>> 5 struct strbuf
>> 5 char
>> 4 struct note_data
>> 3 struct string_list
>> 2 struct strvec
>>
>> Increasing the size of the struct like that would increase the total
>> memory footprint by a few KB at most -- tolerable.
>
> Hmm, I was thinking that "int_value" might not be so bad. But it seems
> like a pretty bad layering violation for parse-options.c to know about
> "struct apply_state" and so on. That really is what void pointers are
> for.
True, keeping a list of external types in parse-options.h is clumsy and
ugly.
> As for size, you should be able to use a union. All of the types inside
> the struct are pointers, so they'd all be the same size. Of course then
> you lose some of the safety. If the caller assigned to "int_value" that
> is distinct from "foo_value", then you can be sure you will get a NULL
> and segfault upon accessing foo_value. With a union, you get whatever
> type-punning undefined behavior the compiler sees fit. And the point is
> making sure the two match.
Which makes a union a non-starter -- we need a reliable error.
> We really don't care about separate storage, though. We want type
> safety. Maybe an option there would be to let the callback function take
> the appropriate type, and cast it. I.e., something like:
>
> /* define a callback taking the real type */
> int my_int_opt(int *value, struct option *opt, ...etc...) { ...body... }
>
> /* when mentioning a callback, include the type, and make sure that
> * the value and the callback both match it */
> #define OPT_CALLBACK(s, l, type, v, cb, ...etc...) \
> { ..., \
> value.type = (v), \
> callback = (int (*)(type *, struct option *opt, ...etc...), \
> }
> ...
> OPT_CALLBACK('f', "foo", int, &my_local_int, my_int_opt)
The idea is good, even though I don't understand how your specific
example would work. Dabbled with something similar for typed qsort
(need to clean it up and submit it one of these days).
> I'm pretty sure that falls afoul of the standard, though, because we
> eventually need to call that function, and we'll do so through a
> function pointer that doesn't match its declaration.
Fighting possible type mismatches by adding a certain type mismatch
won't fly..
> I'm not sure there's a portable and non-insane way of doing what we want
> here. At least at compile-time.
We need a wrapper with the correct signature. The wrapper is plugged
into struct option. The typed callback is called by the wrapper and
can be used for a type check in the struct macro. Demo patch below.
> At run-time you could record type
> information in an enum and check it in the callback. That seems like
> a lot of work for little reward, though.
Agreed -- runtime type checks are for scripting languages..
René
diff --git a/builtin/describe.c b/builtin/describe.c
index b28a4a1f82..ce16c36de2 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -558,15 +558,17 @@ static void describe(const char *arg, int last_one)
strbuf_release(&sb);
}
-static int option_parse_exact_match(const struct option *opt, const char *arg,
- int unset)
+static int option_parse_exact_match(const struct option *opt UNUSED,
+ const char *arg, int unset, int *value)
{
BUG_ON_OPT_ARG(arg);
- max_candidates = unset ? DEFAULT_CANDIDATES : 0;
+ *value = unset ? DEFAULT_CANDIDATES : 0;
return 0;
}
+DEFINE_PARSE_OPT_CB(option_parse_exact_match);
+
int cmd_describe(int argc, const char **argv, const char *prefix)
{
int contains = 0;
@@ -578,9 +580,9 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
OPT_BOOL(0, "long", &longformat, N_("always use long format")),
OPT_BOOL(0, "first-parent", &first_parent, N_("only follow first parent")),
OPT__ABBREV(&abbrev),
- OPT_CALLBACK_F(0, "exact-match", NULL, NULL,
- N_("only output exact matches"),
- PARSE_OPT_NOARG, option_parse_exact_match),
+ OPT_CALLBACK_F_T(0, "exact-match", &max_candidates, NULL,
+ N_("only output exact matches"),
+ PARSE_OPT_NOARG, option_parse_exact_match),
OPT_INTEGER(0, "candidates", &max_candidates,
N_("consider <n> most recent tags (default: 10)")),
OPT_STRING_LIST(0, "match", &patterns, N_("pattern"),
diff --git a/parse-options.h b/parse-options.h
index 57a7fe9d91..f957931cfa 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -67,6 +67,14 @@ enum parse_opt_result {
struct option;
typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
+#define DEFINE_PARSE_OPT_CB(name) \
+static inline int name ## __void(const struct option *opt, \
+ const char *arg, int unset) \
+{ \
+ return name(opt, arg, unset, opt->value); \
+} \
+struct option
+
struct parse_opt_ctx_t;
typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
const struct option *opt,
@@ -198,6 +206,16 @@ struct option {
.flags = (f), \
.callback = (cb), \
}
+#define OPT_CALLBACK_F_T(s, l, v, a, h, f, cb) { \
+ .type = OPTION_CALLBACK, \
+ .short_name = (s) + (0 ? cb(NULL, NULL, 0, (v)) : 0), \
+ .long_name = (l), \
+ .value = (v), \
+ .argh = (a), \
+ .help = (h), \
+ .flags = (f), \
+ .callback = cb ## __void, \
+}
#define OPT_STRING_F(s, l, v, a, h, f) { \
.type = OPTION_STRING, \
.short_name = (s), \
