Re: [PATCH 1/6] parse-options: add precision handling for PARSE_OPT_CMDMODE

From: René Scharfe <l.s.r@web.de>
Date: Jul 1, 2025, 15:15 UTC
Message-ID: <e75c8da8-a859-4980-bbd7-1fb5b03a8113@web.de>
In-Reply-To: <aGO-i6V6_iN678_7@pks.im>

On 7/1/25 12:55 PM, Patrick Steinhardt wrote:

Show 20 quoted lines

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

Show 18 quoted lines

> 
>> @@ -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é