| Message ID | 20230831212128.GF949469@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | more unused parameters in parseopt callbacks | expand |
Am 31.08.23 um 23:21 schrieb Jeff King: > 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. :) > > 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. > 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. > -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. > @@ -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.) > + 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. > 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. > 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. > 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. > 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. > 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? > 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é
On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote: > Am 31.08.23 um 23:21 schrieb Jeff King: > > 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. :) Well, I'm not necessarily _opposed_. :) Mostly my cutoff was cases where the end result was not obviously and immediately more readable. It is not that big a deal to add an UNUSED annotation. My main goal was to use the opportunity to check that we aren't papering over an obvious bug. So for example... > > 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. ...in many cases things can be simplified by parsing into a string, and then validating or acting on the string later. And sometimes that's even a better strategy (because it lets the arg parsing handle all the "last one wins" logic and we just get the result). But it can also make the code harder to follow, too, because it's now split it into segments (although one is mostly declarative, which is nice). So I generally tried to err on the side of not touching working code. Both to avoid breaking it, but also to keep the focus on the goal of the series. And I think that applies to a lot of the other cases you mentioned below (I won't respond to each; I think some of them could be fine, but it also feels like writing and review effort for not much gain. I'm not opposed if you want to dig into them, though). > And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION) > on error, but that's a different matter. Yeah, that doesn't make sense at all. > > -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? Yes, I set diff.interhunkcontext=1 in my config (and have for many years; I think this is the first time anybody noticed in a patch). My rationale is that with "1" the output is never longer, since you save the hunk header. It is a little funny in this case, since the two changes aren't really connected. > 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. I do sometimes use options like that to make a patch more readable, if I notice the difference. I didn't happen to in this case (though I don't disagree with you). As a reviewer, I typically apply and run "git show" myself if I want to dig more (or just go directly look at the preimage in my local repo, of course). > > -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. Yeah, I think the handling of "unset" is a bug (similar to the xopts one fixed earlier). -Peff
Am 05.09.23 um 09:05 schrieb Jeff King: > On Sat, Sep 02, 2023 at 12:12:56PM +0200, René Scharfe wrote: > >> And I don't understand why the callback returns 1 (PARSE_OPT_NON_OPTION) >> on error, but that's a different matter. > > Yeah, that doesn't make sense at all. Actually it does: any non-zero return from a callback is interpreted as an error. "return error(...);" would be shorter, but "error(...); return 1;" is not wrong. Sorry for the noise! René
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, 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; } -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; @@ -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, + 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, 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, 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, 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, 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, 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, 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, const char *arg, int unset) { struct object_id oid;
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. 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(-)