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é
