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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 3, 2023, 17:15 UTC
Message-ID: <xmqq4jj71vbj.fsf@gitster.g>
In-Reply-To: <6cb09270-04b9-456e-8d7e-97137e56e9e2@web.de>

René Scharfe <l.s.r@web.de> writes:

> +DEFINE_OPTION_VALUE_TYPE(resume_type, enum resume_type);

These are a bit annoying, but because we need a token that can be ## pasted to form a valid identifier, we cannot help it.

Show 11 quoted lines

> diff --git a/parse-options.c b/parse-options.c
> index e8e076c3a6..63a2247128 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -85,7 +85,7 @@ static enum parse_opt_result opt_command_mode_error(
>  		if (that == opt ||
>  		    !(that->flags & PARSE_OPT_CMDMODE) ||
>  		    that->value != opt->value ||
> -		    that->defval != *(int *)opt->value)
> +		    that->defval != opt->get_value(opt->value))
>  			continue;

So, instead of assuming the pointer stuffed in opt->value member can be dereferenced as inteter pointer, we have the get_value method for the option and invoke it to grab the value, and compare it with the default value.

Show 8 quoted lines

> @@ -122,7 +122,8 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>  	 * is not a grave error, so let it pass.
>  	 */
>  	if ((opt->flags & PARSE_OPT_CMDMODE) &&
> -	    *(int *)opt->value && *(int *)opt->value != opt->defval)
> +	    opt->get_value(opt->value) &&
> +	    opt->get_value(opt->value) != opt->defval)
>  		return opt_command_mode_error(opt, all_opts, flags);

Likewise.

Show 7 quoted lines

> @@ -160,6 +161,10 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>  		*(int *)opt->value = unset ? 0 : opt->defval;
>  		return 0;
>
> +	case OPTION_SET_VALUE:
> +		opt->set_value(opt->value, unset ? 0 : opt->defval);
> +		return 0;

Here we see the previous way in the precontext of this hunk that is used for OPTION_SET_INT, but in the new type-safe-enum world order, that uses OPTION_SET_VALUE, the set_value method should know what to do with the pointer that is in opt->value.

Show 19 quoted lines

> diff --git a/parse-options.h b/parse-options.h
> index 57a7fe9d91..764e7f7896 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -20,6 +20,7 @@ enum parse_opt_type {
>  	OPTION_BITOP,
>  	OPTION_COUNTUP,
>  	OPTION_SET_INT,
> +	OPTION_SET_VALUE,
>  	/* options with arguments (usually) */
>  	OPTION_STRING,
>  	OPTION_INTEGER,
> @@ -158,8 +159,34 @@ struct option {
>  	parse_opt_ll_cb *ll_callback;
>  	intptr_t extra;
>  	parse_opt_subcommand_fn *subcommand_fn;
> +	intptr_t (*get_value)(void *);
> +	void (*set_value)(void *, intptr_t);
>  };

OK.

Show 16 quoted lines

> +#define DEFINE_OPTION_VALUE_TYPE(type_name, type) \
> +static inline intptr_t type_name##__get(void *void_ptr) \
> +{ \
> +	type *ptr = void_ptr; \
> +	return (intptr_t)*ptr; \
> +} \
> +static inline void type_name##__set(void *void_ptr, intptr_t value) \
> +{ \
> +	type *ptr = void_ptr; \
> +	*ptr = (type)value; \
> +} \
> +static inline void *type_name##__check(type *ptr) \
> +{ \
> +	return ptr; \
> +} \
> +static inline void *type_name##__check(type *ptr)

Fun. So a typical pattern is that for "enum foo", the foo__get() is created from the above template and becomes the .get_value method.

Copying from an earlier hunk, the get_value() method is used like so:

> -		    that->defval != *(int *)opt->value)
> +		    that->defval != opt->get_value(opt->value))

We pass opt->value (which is void *) to foo__get(), we have a local variable "enum foo *ptr" and assign it in there, and dereference it. We used to dereference the pointer as if it were a pointer to an integer, so the type of foo__get() could be "int", but because we compare it with the .defval member, which is of type "intptr_t", the return type of the get_value() method being "intptr_t" would make it consistent here. I am not sure why defval need to be "intptr_t", and for the purpose of this topic it would have been cleaner if it were "int", but that is a tangent (probably somebody uses it as the default value for a pointer variable and points it at some default object).

The setter is also reasonable.  An earlier hunk used it like so:

> +		opt->set_value(opt->value, unset ? 0 : opt->defval);

opt->value which is (void *) is assigned to "enum foo *ptr", and using that pointer, "(enum foo)opt->defval" (or 0) is assinged there. Pretty straight-forward.

Show 6 quoted lines

> +DEFINE_OPTION_VALUE_TYPE(int, int);
> +
> +#define OPTION_VALUE(type_name, v) \
> +	.get_value = type_name##__get, \
> +	.set_value = type_name##__set, \
> +	.value = (1 ? (v) : type_name##__check(v))

This is cute. foo__check() is declared to take "enum foo *" and returns it as "void *", but because the condition to the ternary operator is constant "true", it is discarded. The only expected effect is to force the compiler to catch type errors when v is not of type "enum foo *".

Unless it is "void *", I presume? Then foo__check() would be happy, but typically OPTION_VALUE() is used as an implementation detail of OPT_CMDMODE_T() and you are expected to say something like "&variable" for "v" above, so it would be OK (because you cannot have a variable of type "void").

Thanks for a fun read.