René Scharfe <l.s.r@xxxxxx> 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.
