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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 10, 2023, 18:40 UTC
Message-ID: <ZP4NrVeqMtFTLEuf@nand.local>
In-Reply-To: <2d6f3d74-687a-2d40-5c0c-abc396aef80f@web.de>

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.

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.

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

Thanks, Taylor