Re: [PATCH 1/2] parse-options: add int value pointer to struct option
- From
René Scharfe <l.s.r@web.de>
- Date
- Sep 11, 2023, 20:12 UTC
- Message-ID
- <683efb6d-dc41-51ff-f048-7a23ee955e00@web.de>
- In-Reply-To
- <ZP4NrVeqMtFTLEuf@nand.local>
Am 10.09.23 um 20:40 schrieb Taylor Blau:
Show 23 quoted lines
> 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.
Show 6 quoted lines
> 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.
Show 12 quoted lines
>> @@ -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é