Re: [PATCH 7/8] interpret-trailers: mark unused "unset" parameters in option callbacks

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 31, 2023, 17:04 UTC
Message-ID: <xmqq4jkfxhx2.fsf@gitster.g>
In-Reply-To: <20230831071945.GE3197751@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 7 quoted lines

> There are a few parse-option callbacks that do not look at their "unset"
> parameters, but also do not set PARSE_OPT_NONEG. At first glance this
> seems like a bug, as we'd ignore "--no-if-exists", etc.
>
> But they do work fine, because when "unset" is true, then "arg" is NULL.
> And all three functions pass "arg" on to helper functions which do the
> right thing with the NULL.

Yuck.  That is ugly.

> Note that this shortcut would not be correct if any callback used
> PARSE_OPT_NOARG (in which case "arg" would be NULL but "unset" would be
> false). But none of these do.

That is even uglier. Unlike the BUG_ON_OPT_NEG() and BUG_ON_OPT_ARG() that catch discrepancies between options[] flags and the expectation by the callback function, there is no way for us to protect against such mistakes?

> So the code is fine as-is. But we'll want to mark the unused "unset"
> parameters to quiet -Wunused-parameter.

OK.  Thanks.

Show 32 quoted lines

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/interpret-trailers.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index 6aadce6a1e..a5f9adf6b9 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -24,19 +24,19 @@ static enum trailer_if_exists if_exists;
>  static enum trailer_if_missing if_missing;
>  
>  static int option_parse_where(const struct option *opt,
> -			      const char *arg, int unset)
> +			      const char *arg, int unset UNUSED)
>  {
>  	return trailer_set_where(opt->value, arg);
>  }
>  
>  static int option_parse_if_exists(const struct option *opt,
> -				  const char *arg, int unset)
> +				  const char *arg, int unset UNUSED)
>  {
>  	return trailer_set_if_exists(opt->value, arg);
>  }
>  
>  static int option_parse_if_missing(const struct option *opt,
> -				   const char *arg, int unset)
> +				   const char *arg, int unset UNUSED)
>  {
>  	return trailer_set_if_missing(opt->value, arg);
>  }