Re: [PATCH 2/2] parse-options: use and require int pointer for OPT_CMDMODE

["René Scharfe" <l.s.r@web.de>]

Am 18.09.23 um 15:33 schrieb Phillip Wood:
> Hi René
>
> On 18/09/2023 10:28, René Scharfe wrote:
>> Here's a version that preserves the enums by using additional int
>> variables just for the parsing phase.  No tricks.  The diff is long, but
>> most changes aren't particularly complicated and the resulting code is
>> not that ugly.  Except for builtin/am.c perhaps, which changes the
>> command mode value using a callback as well.
>>
>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 202040b62e..00930e2152 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -2270,13 +2270,14 @@ enum resume_type {
>>
>>   struct resume_mode {
>>       enum resume_type mode;
>> +    int mode_int;
>>       enum show_patch_type sub_mode;
>>   };
>>
>>   static int parse_opt_show_current_patch(const struct option *opt, const char *arg, int unset)
>>   {
>>       int *opt_value = opt->value;
>> -    struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode);
>> +    struct resume_mode *resume = container_of(opt_value, struct resume_mode, mode_int);
>>
>>       /*
>>        * Please update $__git_showcurrentpatch in git-completion.bash
>> @@ -2300,12 +2301,12 @@ static int parse_opt_show_current_patch(const struct option *opt, const char *ar
>>                        "--show-current-patch", arg);
>>       }
>>
>> -    if (resume->mode == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>> +    if (resume->mode_int == RESUME_SHOW_PATCH && new_value != resume->sub_mode)
>>           return error(_("options '%s=%s' and '%s=%s' "
>>                          "cannot be used together"),
>>                        "--show-current-patch", "--show-current-patch", arg, valid_modes[resume->sub_mode]);
>>
>> -    resume->mode = RESUME_SHOW_PATCH;
>> +    resume->mode_int = RESUME_SHOW_PATCH;
>>       resume->sub_mode = new_value;
>>       return 0;
>>   }
>
> Having "mode" and "mode_int" feels a bit fragile as only "mode_int"
> is valid while parsing the options but then we want to use "mode".

True.  It feels a bit worse here than for the others because there the
variable is on the stack and here it's in a struct that is passed
around.

> I wonder if we could get Oswald's idea of using callbacks working in
> a reasonably ergonomic way with a couple of macros. We could add an
> new OPTION_SET_ENUM member to "enum parse_opt_type" that would take
> a setter function as well as the usual void *value. To set the value
> it would pass the value pointer and an integer value to the setter
> function. We could change OPT_CMDMODE to use OPTION_SET_ENUM and
> take the name of the enum as well as the integer value we want to set
> for that option. The name of the enum would be used to generate the
> name of the setter callback which would be defined with another
> macro. The macro to generate the setter would look like
>
> #define MAKE_CMDMODE_SETTER(name) \
>     static void parse_cmdmode_ ## name (void * var, int value) {
>         enum name *p = var;
>         *p = value;
>     }
>
> then OPT_CMDMODE would look like
>
> #define OPT_CMDMODE_F(s, l, v, h, n, i, f) { \
>     .type = OPTION_SET_ENUM, \
>     .short_name = (s), \
>     .long_name = (l), \
>     .value = (v), \
>     .help = (h), \
>     .flags = PARSE_OPT_CMDMODE|PARSE_OPT_NOARG|PARSE_OPT_NONEG | (f), \
>     .defval = (i), \
>     .enum_setter = parse_cmdmode_ ## n,
> }
> #define OPT_CMDMODE(s, l, v, h, n, i)  OPT_CMDMODE_F(s, l, v, h, n, i, 0)
>
>
> Then in builtin/am.c at the top level we'd add
>
> MAKE_CMDMODE_SETTER(resume_type)
>
> and change the option definitions to look like
>
> OPT_CMDMODE(0, "continue", resume_type, &resume.mode, ...)

About half of the OPT_CMDMODE users use int, not enum.  They'd have to
be converted.  On the flipside the direct and indirect uses of
OPT_SET_INT_F with enums could be converted to an OPT_SET_ENUM_F based
on the above to avoid their use of incompatible pointers as well (e.g.
OPT_IPVERSION).

For uses of OPT_BIT and OPT_NEGBIT with enums a getter would be needed
as well, though (e.g. git ls-tree -d/-r/-t).

At this point I wonder if the existing callback mechanism would suffice.
Which brings me full circle to the topic of typed callbacks.  Perhaps I
should introduce them first and come back to solving the int/enum issue
at the end of that journey.

René