Re: [PATCH] describe: fix --no-exact-match
- From
René Scharfe <l.s.r@web.de>
- Date
- Aug 11, 2023, 17:59 UTC
- Message-ID
- <fbeadf7d-c16f-0612-6256-fc4355e89638@web.de>
- In-Reply-To
- <20230811151102.GE2303200@coredump.intra.peff.net>
Am 11.08.23 um 17:11 schrieb Jeff King:
Show 20 quoted lines
> On Thu, Aug 10, 2023 at 09:10:33PM +0200, René Scharfe wrote:
>
>>> 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.
>
> OK, clever. So we have two functions, one with a real body, and the
> other which is used with the void pointer. How do we make sure that the
> real-body one matches the type passed to OPT_CALLBACK(), if it is only
> seeing the void wrapper? I guess that is this bit in short_name:
>
>> +#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), \
>
> which would cause the compiler to barf, and presumably eliminate the
> dead code (or at the very least never call it at runtime).Yes, a fake call that doesn't make it into the generated object code, but is real enough for type checking.
Show 13 quoted lines
> So I think that works. Though...
>
>> +#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); \
>> +} \
>
> we are defining an inline function with the explicit goal of passing it
> as a function pointer. I don't remember all of the standard's rules
> here. Are we guaranteed that it will create a linkable version if
> necessary?I don't see on which basis the compiler could refuse. We can't expect the function address to be the same across compilation units, but we don't need that. If there's a compiler that won't do it or a standards verse that makes this dubious then I'd like to know very much.
> We could probably drop the "inline" (and perhaps would need to add > MAYBE_UNUSED in such a case).
Yes, we can use a non-inline function as well, but then we'd need to define it once somewhere and declare it separately, though, which is inconvenient. It needs to have the same visibility as the typed function.
We better not use MAYBE_UNUSED because the wrapper is the actually invoked callback.
Show 31 quoted lines
>> 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);
>
> I wondered about combining these, like:
>
> DEFINE_PARSE_OPT_CB(option_parse_exact_match, int) {
> ...the real body here...
> }
>
> But I guess that may confuse non-compiler parsers, and it doesn't leave
> room for annotations like the UNUSED above (which ironically is still
> needed, since now we pass opt->value as its own parameter).I get the idea. Something that looks like a function attribute or decorator. I don't see no nice way to achieve that, though.
> So I dunno. Clever, for sure, and I think it would work. I'm not sure if > the extra code merits the return or not.
Sure, why check types -- script languages get work done as well. (I'm fresh off a Python basics training, nice quirky language..) But we're in C land and static typing is supposed to help us get our operations correct and portable.
A good example in parseopt: The patch below adds type checking to the int options and yields 79 warning about incompatible pointers, because enum pointers were used in integer option definitions. The storage size of enums depends on the member values and the compiler; an enum could be char-sized. When we access such a thing with an int pointer we write up to seven bytes of garbage ... somewhere. We better fix that.
It also gives me 73 sign warnings because we like to use unsigned int to store int options for some reason. No problem storage-wise. I have to admit that I don't really understand why -Wpointer-sign is in -Wall and not only in -Wpedantic. But it could mean a user gives a negative value for something and we interpret it as a huge value. Probably still worth fixing to avoid the general weirdness.
My point is: If we don't check types we deviate. It just happens.
René
PS: We could simplify the type check like this:
.short_name = (s) + (0 && (v) == (int *)0), \
This yields -Wcompare-distinct-pointer-types for all 152 cases, which may suffice. But comparisons of pointers are only defined for those that point to the same object or just past it if I'm not mistaken, so this way works only by accident and the comparison part of the warning is misleading anyway. Sill looking for a simpler type check..
diff --git a/parse-options.h b/parse-options.h index f957931cfa..dafea28b55 100644 --- a/parse-options.h +++ b/parse-options.h @@ -168,9 +168,14 @@ struct option { parse_opt_subcommand_fn *subcommand_fn; }; +static inline int parse_options_noop_int_ptr(int *p UNUSED) +{ + return 0; +} + #define OPT_BIT_F(s, l, v, h, b, f) { \ .type = OPTION_BIT, \ - .short_name = (s), \ + .short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \ .long_name = (l), \ .value = (v), \ .help = (h), \ @@ -180,7 +185,7 @@ struct option { } #define OPT_COUNTUP_F(s, l, v, h, f) { \ .type = OPTION_COUNTUP, \ - .short_name = (s), \ + .short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \ .long_name = (l), \ .value = (v), \ .help = (h), \ @@ -188,7 +193,7 @@ struct option { } #define OPT_SET_INT_F(s, l, v, h, i, f) { \ .type = OPTION_SET_INT, \ - .short_name = (s), \ + .short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \ .long_name = (l), \ .value = (v), \ .help = (h), \ @@ -227,7 +232,7 @@ struct option { } #define OPT_INTEGER_F(s, l, v, h, f) { \ .type = OPTION_INTEGER, \ - .short_name = (s), \ + .short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \ .long_name = (l), \ .value = (v), \ .argh = N_("n"), \ @@ -245,7 +250,7 @@ struct option { #define OPT_BIT(s, l, v, h, b) OPT_BIT_F(s, l, v, h, b, 0) #define OPT_BITOP(s, l, v, h, set, clear) { \ .type = OPTION_BITOP, \ - .short_name = (s), \ + .short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \ .long_name = (l), \ .value = (v), \ .help = (h), \ @@ -255,7 +260,7 @@ struct option { } #define OPT_NEGBIT(s, l, v, h, b) { \ .type = OPTION_NEGBIT, \ - .short_name = (s), \ + .short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \ .long_name = (l), \ .value = (v), \ .help = (h), \ @@ -267,7 +272,7 @@ struct option { #define OPT_BOOL(s, l, v, h) OPT_BOOL_F(s, l, v, h, 0) #define OPT_HIDDEN_BOOL(s, l, v, h) { \ .type = OPTION_SET_INT, \ - .short_name = (s), \ + .short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \ .long_name = (l), \ .value = (v), \ .help = (h), \ @@ -276,7 +281,7 @@ struct option { } #define OPT_CMDMODE_F(s, l, v, h, i, f) { \ .type = OPTION_SET_INT, \ - .short_name = (s), \ + .short_name = (s) + (0 ? parse_options_noop_int_ptr((v)) : 0), \ .long_name = (l), \ .value = (v), \ .help = (h), \