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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2023, 17:11 UTC
Message-ID: <xmqqedivl832.fsf@gitster.g>
In-Reply-To: <0bf56c65-e59f-4290-8160-cce141f692d5@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 24 quoted lines

>> -	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". 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;
> 	}

Ah, OK. So that's how you defeat "how the size and alignment of an enum mixes well with int is not known and depends on particular enum type". It is a tad sad that this relies on "void *", which means that the caller of parse_cmdmode_resume_type cannot be forced by the compilers to pass "enum resume_type *" to the function, though. And that is probably inevitable with the design as .enum_setter needs to be of a single type, and the member in the "struct option" that points at the destination variable must be "void *" as it has to be capable of pointing at various different enum types.

Show 8 quoted lines

> ...
> 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, ...)

Yup, that is ergonomic and corrects "The shape of a particular enum may not match 'int'" issue nicely. I do not know how severe the problem is that it is not quite type safe that we cannot enforce resume_type is the same as typeof(resume.mode) here, though.

Thanks.