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

[Junio C Hamano <gitster@pobox.com>]

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.

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

> @@ -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.

> @@ -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.

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

> +#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.

> +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.