| Message ID | 20230831212220.GJ949469@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0058b3d5eedcf5777712e872e01f74bf8d933be7 |
| Headers | show |
| Series | more unused parameters in parseopt callbacks | expand |
Am 31.08.23 um 23:22 schrieb Jeff King: > Unsurprisingly, the noop options callback doesn't bother to look at any > of its parameters. Let's mark them so that -Wunused-parameter does not > complain. > > Another option would be to drop the callback and have parse-options > itself recognize OPT_NOOP_NOARG. But that seems like extra work for no > real benefit. I'm not sure about that -- we don't need to add special flags or option types to drop parse_opt_noop_cb(). > > Signed-off-by: Jeff King <peff@peff.net> > --- > parse-options-cb.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/parse-options-cb.c b/parse-options-cb.c > index a24521dee0..bdc7fae497 100644 > --- a/parse-options-cb.c > +++ b/parse-options-cb.c > @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset) > return 0; > } > > -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset) > +int parse_opt_noop_cb(const struct option *opt UNUSED, > + const char *arg UNUSED, > + int unset UNUSED) > { > return 0; > } Your patch is simple and makes sense. The one below is more complicated, but leaves the code in a slightly better state. I'd go with that variant if I'd had to add OPT_NOOP_NOARG today, and I slightly prefer it, but similar to you think that the benefit is low (though non-zero). So I'm unsure if that's enough to be worth it or just bikeshedding with some hindsight.. René --- >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. 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. Signed-off-by: René Scharfe <l.s.r@web.de> --- parse-options-cb.c | 5 ----- parse-options.c | 7 +++---- parse-options.h | 2 -- 3 files changed, 3 insertions(+), 11 deletions(-) diff --git a/parse-options-cb.c b/parse-options-cb.c index a24521dee0..e79cd54ec2 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -227,11 +227,6 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset) return 0; } -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset) -{ - return 0; -} - /** * Recreates the command-line option in the strbuf. */ diff --git a/parse-options.c b/parse-options.c index 76d2e76b49..5be8de93f5 100644 --- a/parse-options.c +++ b/parse-options.c @@ -201,8 +201,9 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, } if (opt->callback) return (*opt->callback)(opt, p_arg, p_unset) ? (-1) : 0; - else + if (opt->ll_callback) return (*opt->ll_callback)(p, opt, p_arg, p_unset); + return 0; } case OPTION_INTEGER: if (unset) { @@ -494,9 +495,7 @@ static void parse_options_check(const struct option *opts) optbug(opts, "should not accept an argument"); break; case OPTION_CALLBACK: - if (!opts->callback && !opts->ll_callback) - optbug(opts, "OPTION_CALLBACK needs one callback"); - else if (opts->callback && opts->ll_callback) + if (opts->callback && opts->ll_callback) optbug(opts, "OPTION_CALLBACK can't have two callbacks"); break; case OPTION_LOWLEVEL_CALLBACK: diff --git a/parse-options.h b/parse-options.h index 57a7fe9d91..41bb47120d 100644 --- a/parse-options.h +++ b/parse-options.h @@ -348,7 +348,6 @@ struct option { .long_name = (l), \ .help = N_("no-op (backward compatibility)"), \ .flags = PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, \ - .callback = parse_opt_noop_cb, \ } #define OPT_ALIAS(s, l, source_long_name) { \ @@ -490,7 +489,6 @@ int parse_opt_commit(const struct option *, const char *, int); int parse_opt_tertiary(const struct option *, const char *, int); int parse_opt_string_list(const struct option *, const char *, int); int parse_opt_strvec(const struct option *, const char *, int); -int parse_opt_noop_cb(const struct option *, const char *, int); int parse_opt_passthru(const struct option *, const char *, int); int parse_opt_passthru_argv(const struct option *, const char *, int); /* value is enum branch_track* */ -- 2.42.0
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
Am 05.09.23 um 09:09 schrieb Jeff King: > > 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, Fair enough. > 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. Well, my aim is more to simplify and improve type safety, but I get carried away a bit at times. René
diff --git a/parse-options-cb.c b/parse-options-cb.c index a24521dee0..bdc7fae497 100644 --- a/parse-options-cb.c +++ b/parse-options-cb.c @@ -227,7 +227,9 @@ int parse_opt_strvec(const struct option *opt, const char *arg, int unset) return 0; } -int parse_opt_noop_cb(const struct option *opt, const char *arg, int unset) +int parse_opt_noop_cb(const struct option *opt UNUSED, + const char *arg UNUSED, + int unset UNUSED) { return 0; }
Unsurprisingly, the noop options callback doesn't bother to look at any of its parameters. Let's mark them so that -Wunused-parameter does not complain. Another option would be to drop the callback and have parse-options itself recognize OPT_NOOP_NOARG. But that seems like extra work for no real benefit. Signed-off-by: Jeff King <peff@peff.net> --- parse-options-cb.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)