On 7/1/25 12:55 PM, Patrick Steinhardt wrote:
> On Sun, Jun 29, 2025 at 01:50:31PM +0200, René Scharfe wrote:
>> Build on 09705696f7 (parse-options: introduce precision handling for
>> `OPTION_INTEGER`, 2025-04-17) to support value variables of different
>> sizes for PARSE_OPT_CMDMODE options. Do that by requiring their
>> "precision" to be set and casting their "value" pointer accordingly.
>
> Makes sense.
>
>> get_value() needs to access all PARSE_OPT_CMDMODE values in addition to
>> the actual value it is supposed to get to detect conflicting changes.
>> Give it an example struct option pointer in cmdmode_list instead of just
>> the "value" pointer to allow it to use the proper "precision".
>>
>> Use optbug() in get_int_value() to report options with unsupported
>> "precision" values without requiring enum opt_parsed flags, as we don't
>> have them in build_cmdmode_list(). Use BUG right afterwards to abort
>> for uses outside of build_cmdmode_list() by aborting immediately.
>
> Hm. I have a bit of a hard time understanding these two paragraphs, to
> be honest. Might be that my brain is still in vacation mode.
Or it might be my needing-a-vacation mode.
get_value() checks all PARSE_OPT_CMDMODE value variables. It does that
to detect changes to such a variable by non-PARSE_OPT_CMDMODE options.
"All" sounds grand, but actual commands only have at most a single
PARSE_OPT_CMDMODE value variable. But they could have many more.
Anyway, the patch gives it a struct option so that it can use the
proper precision when dereferencing value, a void pointer.
optbug() calls the macro "bug". It reports a bug just like BUG, but
does not abort. It's intended for cases where we want to report all the
bugs that we can find instead of forcing developers to fix them one by
one and recompile in between. That would be nice in
build_cmdmode_list(). I still force an abort by following it with BUG,
though, because handling invalid "precision" values would be tedious
in the other callers.
>
>> @@ -280,19 +297,18 @@ static void build_cmdmode_list(struct parse_opt_ctx_t *ctx,
>>
>> for (; opts->type != OPTION_END; opts++) {
>> struct parse_opt_cmdmode_list *elem = ctx->cmdmode_list;
>> - int *value_ptr = opts->value;
>>
>> - if (!(opts->flags & PARSE_OPT_CMDMODE) || !value_ptr)
>> + if (!(opts->flags & PARSE_OPT_CMDMODE) || !opts->value)
>> continue;
>>
>> - while (elem && elem->value_ptr != value_ptr)
>> + while (elem && elem->reference_opt->value != opts->value)
>> elem = elem->next;
>
> Hm. Previously we checked for the pointers to be equal, now we check for
> the value to be equal. Are we sure that this is always equivalent? Can't
> it ever be that two elements might have the same value?
The "value" member of struct option is a void pointer. We still compare
pointers here.
René
