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@xxxxxxxx>
> ---
> 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@xxxxxx>
---
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
