Re: [PATCH v2 10/10] parse-options: mark unused parameters in noop callback
- From
René Scharfe <l.s.r@web.de>
- Date
- Sep 2, 2023, 11:37 UTC
- Message-ID
- <3bf1bd4a-9d28-32c1-7838-09acd2c85d03@web.de>
- In-Reply-To
- <20230831212220.GJ949469@coredump.intra.peff.net>
Am 31.08.23 um 23:22 schrieb Jeff King:
Show 7 quoted lines
> 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().
Show 21 quoted lines
>
> 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