Jeff King <peff@xxxxxxxx> writes:
> On Fri, Jul 21, 2023 at 03:41:33PM +0200, René Scharfe wrote:
>
>> +static int option_parse_exact_match(const struct option *opt, const char *arg,
>> + int unset)
>> +{
>> + BUG_ON_OPT_ARG(arg);
>> +
>> + max_candidates = unset ? DEFAULT_CANDIDATES : 0;
>> + return 0;
>> +}
>
> I wanted to call out a style question here. The "opt" parameter is
> unused, since it manipulates the "max_candidates" global directly. I can
> add an UNUSED annotation to satisfy -Wunused-parameter, but in such
> cases I've usually been modifying them like:
>
> int *value = opt->value;
> ...
> *value = unset ? DEFAULT_CANDIDATES : 0;
>
> so that the callback operates on the value passed in the options list.
Yeah, I wasn't paying attention to that particular detail, but I
tend to think that using opt->value to point at the variable would
make sense here. That approach matches what is internally done by
OPT_STRING_LIST() and OPT_STRVEC(), the built-in types that are
implemented via the same OPTION_CALLBACK mechanism.
One good thing about it is that it makes it clear, without looking
at the implementation of the callback function, which variables are
affected only by looking at what OPT_CALLBACK() says.
> So before I sent a patch (either to switch to using opt->value, or to
> add an UNUSED annotation), I wanted to see what you (or others) thought
> between the two. I.e., should we have a rule of "try not to operate on
> global data via option callbacks" or is that just being too pedantic for
> one-off callbacks like this?
So, that was my preference, but I may be missing some obvious
downsides. I am interested in hearing from René, who often shows
better taste than I do in these cases ;-)
Thanks.
