Re: [PATCH 1/2] parse-options: add int value pointer to struct option

["René Scharfe" <l.s.r@web.de>]

Am 10.09.23 um 20:40 schrieb Taylor Blau:
> On Sat, Sep 09, 2023 at 11:10:36PM +0200, René Scharfe wrote:
>> Add an int pointer, value_int, to struct option to provide a typed value
>> pointer for the various integer options.  It allows type checks at
>> compile time, which is not possible with the void pointer, value.  Its
>> use is optional for now.
>
> This is an interesting direction. I wonder about whether or not you'd
> consider changing the option structure to contain a tagged union type
> that represents some common cases we'd want from a parse-options
> callback, something like:
>
>     struct option {
>         /* ... */
>         union {
>             void *value;
>             int *value_int;
>             /* etc ... */
>         } u;
>         enum option_type t;
>     };
>
> where option_type has some value corresponding to "void *", another for
> "int *", and so on.

In a hand-made struct option this would only provide a very limited form
of type safety.  It reduces the number of incorrect types to choose from
from basically infinity to a handful, but still allows pointing the
union e.g. to an int for an option that takes a long or a string without
any compiler warning or error.

Convenience macros like OPT_CMDMODE could use the union to provide a
type safe interface, though, true.  This might suffice for our purposes.

> Alternatively, perhaps you are thinking that we'd use both the value
> pointer and the value_int pointer to point at potentially different
> values in the same callback. I don't have strong feelings about it, but
> I'd just as soon encourage us to shy away from that approach, since
> assigning a single callback parameter to each function seems more
> organized.

Right, we only need one active value pointer per option.

>> @@ -109,6 +110,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
>>  	const char *s, *arg;
>>  	const int unset = flags & OPT_UNSET;
>>  	int err;
>> +	int *value_int = opt->value_int ? opt->value_int : opt->value;
>>
>>  	if (unset && p->opt)
>>  		return error(_("%s takes no value"), optname(opt, flags));
>
> Reading this hunk, I wonder whether we even need a type tag (the
> option_type enum above) if each callback knows a priori what type it
> expects. But I think storing them together in a union makes sense to do.

Yes, option types (OPTION_INTEGER etc.) already imply a pointer type,
no additional tag needed.

René