git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 06/10] parse-options: mark unused "opt" parameter in callbacks

From
René Scharfe <l.s.r@web.de>
Date
Sep 2, 2023, 10:12 UTC
Message-ID
<98d1cd21-fb2a-269a-8d0b-f3e050682739@web.de>
In-Reply-To
<20230831212128.GF949469@coredump.intra.peff.net>
Am 31.08.23 um 23:21 schrieb Jeff King:
Show 9 quoted lines
> The previous commit argued that parse-options callbacks should try to
> use opt->value rather than touching globals directly. In some cases,
> however, that's awkward to do. Some callbacks touch multiple variables,
> or may even just call into an abstracted function that does so.
>
> In some of these cases we _could_ convert them by stuffing the multiple
> variables into a single struct and passing the struct pointer through
> opt->value. But that may make other parts of the code less readable,
> as the struct relationship has to be mentioned everywhere.

Does that imply you'd be willing to use other methods? Let's find out below. :)

Show 24 quoted lines
>
> Let's just accept that these cases are special and leave them as-is. But
> we do need to mark their "opt" parameters to satisfy -Wunused-parameter.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/gc.c           |  2 +-
>  builtin/log.c          | 10 ++++++----
>  builtin/merge.c        |  2 +-
>  builtin/pack-objects.c |  6 +++---
>  builtin/read-tree.c    |  2 +-
>  builtin/update-index.c |  4 ++--
>  6 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 369bd43fb2..b842349d86 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -1403,7 +1403,7 @@ static void initialize_task_config(int schedule)
>  	strbuf_release(&config_name);
>  }
>
> -static int task_option_parse(const struct option *opt,
> +static int task_option_parse(const struct option *opt UNUSED,

Only the global variable "tasks" seems to be used in here if you don't count the constant "TASK__COUNT", so you could pass it in. This could also be converted to OPT_STRING_LIST with parsing and duplicate checking done later.

And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION) on error, but that's a different matter.

Show 22 quoted lines
>  			     const char *arg, int unset)
>  {
>  	int i, num_selected = 0;
> diff --git a/builtin/log.c b/builtin/log.c
> index fb90d43717..3599063554 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -118,16 +118,17 @@ static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
>  static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP;
>  static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP;
>
> -static int clear_decorations_callback(const struct option *opt,
> -					    const char *arg, int unset)
> +static int clear_decorations_callback(const struct option *opt UNUSED,
> +				      const char *arg, int unset)
>  {
>  	string_list_clear(&decorate_refs_include, 0);
>  	string_list_clear(&decorate_refs_exclude, 0);
>  	use_default_decoration_filter = 0;
>  	return 0;
>  }
>
Meta: Why do we get seven lines of context in an -U3 patch here?  Did
you use --inter-hunk-context?

This patch would be better viewed with --function-context to see that the callbacks change multiple variables or do other funky things. Only doubles the line count.

Show 6 quoted lines
> -static int decorate_callback(const struct option *opt, const char *arg, int unset)
> +static int decorate_callback(const struct option *opt UNUSED, const char *arg,
> +			     int unset)
>  {
>  	if (unset)
>  		decoration_style = 0;

Sets decoration_style and decoration_given. Never sets decoration_given to a negative value. This could be used to initialize decoration_style to -1 and set decoration_given after parsing, then you could pass in just decoration_style.

Show 6 quoted lines
> @@ -1555,7 +1556,8 @@ static int inline_callback(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>
> -static int header_callback(const struct option *opt, const char *arg, int unset)
> +static int header_callback(const struct option *opt UNUSED, const char *arg,

Sorts strings into three string_lists based on prefix. Could become a vanilla OPT_STRING_LIST with parsing and sorting done later, if it wouldn't interact with to_callback() and cc_callback().

(--function-context is not enough to see that, it requires a look at the definition of those other functions as well.)

Show 14 quoted lines
> +			   int unset)
>  {
>  	if (unset) {
>  		string_list_clear(&extra_hdr, 0);
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 21363b7985..0436986dab 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -231,7 +231,7 @@ static void append_strategy(struct strategy *s)
>  	use_strategies[use_strategies_nr++] = s;
>  }
>
> -static int option_parse_strategy(const struct option *opt,
> +static int option_parse_strategy(const struct option *opt UNUSED,

Could be an OPT_STRING_LIST and parsing done later. Except that --no-strategy does nothing, which is weird.

Show 13 quoted lines
>  				 const char *name, int unset)
>  {
>  	if (unset)
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 492372ee5d..91b4b7c177 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3739,7 +3739,7 @@ static void show_object__ma_allow_promisor(struct object *obj, const char *name,
>  	show_object(obj, name, data);
>  }
>
> -static int option_parse_missing_action(const struct option *opt,
> +static int option_parse_missing_action(const struct option *opt UNUSED,

Sets the enum arg_missing_action, fetch_if_missing and fn_show_object. Could be changed to set only the enum and then you could pass it in.

Show 9 quoted lines
>  				       const char *arg, int unset)
>  {
>  	assert(arg);
> @@ -4150,7 +4150,7 @@ static int option_parse_index_version(const struct option *opt,
>  	return 0;
>  }
>
> -static int option_parse_unpack_unreachable(const struct option *opt,
> +static int option_parse_unpack_unreachable(const struct option *opt UNUSED,

Sets both unpack_unreachable and unpack_unreachable_expiration. Perhaps could set only unpack_unreachable_expiration and use TIME_MAX if no argument is given, then set unpack_unreachable after parsing? Not sure.

Show 9 quoted lines
>  					   const char *arg, int unset)
>  {
>  	if (unset) {
> @@ -4165,7 +4165,7 @@ static int option_parse_unpack_unreachable(const struct option *opt,
>  	return 0;
>  }
>
> -static int option_parse_cruft_expiration(const struct option *opt,
> +static int option_parse_cruft_expiration(const struct option *opt UNUSED,

Does the same as option_parse_unpack_unreachable(), just with two different variables. And interacts with --cruft.

Show 13 quoted lines
>  					 const char *arg, int unset)
>  {
>  	if (unset) {
> diff --git a/builtin/read-tree.c b/builtin/read-tree.c
> index 1fec702a04..8196ca9dd8 100644
> --- a/builtin/read-tree.c
> +++ b/builtin/read-tree.c
> @@ -49,7 +49,7 @@ static const char * const read_tree_usage[] = {
>  	NULL
>  };
>
> -static int index_output_cb(const struct option *opt, const char *arg,
> +static int index_output_cb(const struct option *opt UNUSED, const char *arg,

Calls set_alternate_index_output(). Could become an OPT_STRING_F if we assume that the last value wins and earlier calls have no side effects.

Show 13 quoted lines
>  				 int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index aee3cb8cbd..59acae3336 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -856,7 +856,7 @@ static int chmod_callback(const struct option *opt,
>  	return 0;
>  }
>
> -static int resolve_undo_clear_callback(const struct option *opt,
> +static int resolve_undo_clear_callback(const struct option *opt UNUSED,

Affects the_index. This option (--clear-resolve-undo) is not mentioned in Documentation/, by the way. Not sure if it interacts with other callbacks, like the one below. Otherwise could be an OPT_BOOL and its action done after parsing. Perhaps pass in &the_index?

Show 9 quoted lines
>  				const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> @@ -890,7 +890,7 @@ static int parse_new_style_cacheinfo(const char *arg,
>  }
>
>  static enum parse_opt_result cacheinfo_callback(
> -	struct parse_opt_ctx_t *ctx, const struct option *opt,
> +	struct parse_opt_ctx_t *ctx, const struct option *opt UNUSED,

A low-level callback for a change. Affects the_index. Pass it in? add_cacheinfo() would need to grow a parameter to receive it. It would document that this option directly changes the_index.

>  	const char *arg, int unset)
>  {
>  	struct object_id oid;
Phew, looks like that's stuff for a whole new series or two!
René
Previous: Jeff KingNext: Jeff King
Message 38 of 47 in “more unused parameters in parseopt callbacks”
  1. 0/8 more unused parameters in parseopt callbacksJeff King, Aug 31, 2023
  2. 1/8 merge: make xopts a strvecJeff King, Aug 31, 2023
  3. Jeff KingAug 31, 2023
  4. Phillip WoodAug 31, 2023
  5. Junio C HamanoAug 31, 2023
  6. Taylor BlauAug 31, 2023
  7. 2/8 merge: simplify parsing of "-n" optionJeff King, Aug 31, 2023
  8. Junio C HamanoAug 31, 2023
  9. 3/8 parse-options: prefer opt->value to globals in callbacksJeff King, Aug 31, 2023
  10. Junio C HamanoAug 31, 2023
  11. 4/8 parse-options: mark unused "opt" parameter in callbacksJeff King, Aug 31, 2023
  12. Junio C HamanoAug 31, 2023
  13. Jeff KingAug 31, 2023
  14. Jeff KingAug 31, 2023
  15. 5/8 merge: do not pass unused opt->value parameterJeff King, Aug 31, 2023
  16. Junio C HamanoAug 31, 2023
  17. 6/8 parse-options: add more BUG_ON() annotationsJeff King, Aug 31, 2023
  18. Junio C HamanoAug 31, 2023
  19. 7/8 interpret-trailers: mark unused "unset" parameters in option callbacksJeff King, Aug 31, 2023
  20. Junio C HamanoAug 31, 2023
  21. Jeff KingAug 31, 2023
  22. 8/8 parse-options: mark unused parameters in noop callbackJeff King, Aug 31, 2023
  23. Junio C HamanoAug 31, 2023
  24. 0/10 more unused parameters in parseopt callbacksJeff King, Aug 31, 2023
  25. 01/10 merge: make xopts a strvecJeff King, Aug 31, 2023
  26. 02/10 merge: simplify parsing of "-n" optionJeff King, Aug 31, 2023
  27. René ScharfeSep 2, 2023
  28. Jeff KingSep 5, 2023
  29. 03/10 format-patch: use OPT_STRING_LIST for to/cc optionsJeff King, Aug 31, 2023
  30. 04/10 checkout-index: delay automatic setting of to_tempfileJeff King, Aug 31, 2023
  31. Junio C HamanoAug 31, 2023
  32. René ScharfeSep 2, 2023
  33. 04/10 checkout-index: delay automatic setting of to_tempfileJeff King, Sep 5, 2023
  34. 05/10 parse-options: prefer opt->value to globals in callbacksJeff King, Aug 31, 2023
  35. René ScharfeSep 2, 2023
  36. Jeff KingSep 5, 2023
  37. 06/10 parse-options: mark unused "opt" parameter in callbacksJeff King, Aug 31, 2023
  38. René ScharfeSep 2, 2023
  39. Jeff KingSep 5, 2023
  40. René ScharfeSep 19, 2023
  41. 07/10 merge: do not pass unused opt->value parameterJeff King, Aug 31, 2023
  42. 08/10 parse-options: add more BUG_ON() annotationsJeff King, Aug 31, 2023
  43. 09/10 interpret-trailers: mark unused "unset" parameters in option callbacksJeff King, Aug 31, 2023
  44. 10/10 parse-options: mark unused parameters in noop callbackJeff King, Aug 31, 2023
  45. René ScharfeSep 2, 2023
  46. Jeff KingSep 5, 2023
  47. René ScharfeSep 7, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.