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

["René Scharfe" <l.s.r@web.de>]

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