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

From: Jeff King <peff@peff.net>
Date: Sep 5, 2023, 07:09 UTC
Message-ID: <20230905070948.GD199565@coredump.intra.peff.net>
In-Reply-To: <3bf1bd4a-9d28-32c1-7838-09acd2c85d03@web.de>

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

Show 11 quoted lines

> --- >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 *").

Show 7 quoted lines

> 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