Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback

[Jeff King <peff@peff.net>]

On Sat, Sep 02, 2023 at 01:37:08PM +0200, René Scharfe wrote:

> --- >8 ---
> Subject: [PATCH] parse-options: let NULL callback be a noop
> 
> Allow OPTION_CALLBACK options to have a NULL callback function pointer
> and do nothing in that case, yet still handle arguments as usual.  Use
> this to replace parse_opt_noop_cb().
> 
> We lose the ability to distinguish between a forgotten function pointer
> and intentional noop, but that will be caught by a compiler warning
> about an unused function in many cases and having an option do nothing
> is a benign failure mode.

Yeah, my concern would be missing an accidental NULL here. I guess that
is pretty unlikely in practice, though. I'd guess the worst case would
be somebody accidentally putting the function into the "opt->value" slot
where the compiler might then think it is used (though I don't know
off-hand if it would complain about an implicit conversion of a function
pointer into a "void *").

> We also lose the ability to add a warning to the callback, but we
> haven't done that since it was added by 6acec0380b (parseopt: add
> OPT_NOOP_NOARG, 2011-09-28), so we probably won't do it soon.  We can
> add a callback back when we want to show a warning.
> 
> By letting go of features we don't need this patch simplifies the
> parse-options API and gets rid of an exported empty function.

Your patch looks correct to me. I probably wouldn't have bothered to
write it, but now you did. :) My inclination would be to go with my
dumb-simple one for this series, and it looks like you may have
collected a few slight-more-risky-but-maybe-worthwhile parseopt cleanups
on top that could make their own series.

-Peff