| Message ID | 20230831071820.GB3197751@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | more unused parameters in parseopt callbacks | expand |
Jeff King <peff@peff.net> writes: > diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c > index f62f13f2b5..95b3717dd1 100644 > --- a/builtin/checkout-index.c > +++ b/builtin/checkout-index.c > @@ -190,7 +190,7 @@ static const char * const builtin_checkout_index_usage[] = { > NULL > }; > > -static int option_parse_stage(const struct option *opt, > +static int option_parse_stage(const struct option *opt UNUSED, > const char *arg, int unset) > { > BUG_ON_OPT_NEG(unset); I suspect that the original is buggy; when given $ git checkout-index --stage=all --stage=1 path the first option turns --temp on, but the second one does not turn it off. For now I think being bug-to-bug compatible and annotating the opt as UNUSED is good, but as a follow-up, we could make the caller: (1) point &checkout_stage with opt->value; (2) make to_tempfile to tristate <unspecified, false, true> by initializing it to -1; (3) adjust to_tempfile that is still <unspecified> after parse_options() returns, according to the value in checkout_stage. and then this can follow the "opt->value points at the variable that is affected" pattern. > diff --git a/builtin/fetch.c b/builtin/fetch.c > index 8f93529505..cd2afe33e5 100644 > --- a/builtin/fetch.c > +++ b/builtin/fetch.c > @@ -168,7 +168,8 @@ static int git_fetch_config(const char *k, const char *v, > return git_default_config(k, v, ctx, cb); > } > > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset) > +static int parse_refmap_arg(const struct option *opt UNUSED, > + const char *arg, int unset) > { > BUG_ON_OPT_NEG(unset); Can't this just point opt->value at the global &refmap? Obviously not a huge deal, as we could have taken the "annotate as UNUSED" approach for all the functions in [3/8]. > 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; The TASK__COUNT constant is very closely tied to the task[] array and it is not worth passing the address of the array in opt->value. The loss of clarity inside the callback funtion is a more serious downside than the value on the caller side to document what array is being modified. So I agree this is a good change. > 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; These two refuses to take "arg" and the callback mechanism is used purely to trigger the side effect of the function, not as a part of parsing something for its "value", so I agree that these UNUSED annotations reflect the reality very well. Thanks.
On Thu, Aug 31, 2023 at 09:33:22AM -0700, Junio C Hamano wrote: > > -static int option_parse_stage(const struct option *opt, > > +static int option_parse_stage(const struct option *opt UNUSED, > > const char *arg, int unset) > > { > > BUG_ON_OPT_NEG(unset); > > I suspect that the original is buggy; when given > > $ git checkout-index --stage=all --stage=1 path > > the first option turns --temp on, but the second one does not turn > it off. For now I think being bug-to-bug compatible and annotating > the opt as UNUSED is good, but as a follow-up, we could make the > caller: > > (1) point &checkout_stage with opt->value; > > (2) make to_tempfile to tristate <unspecified, false, true> by > initializing it to -1; > > (3) adjust to_tempfile that is still <unspecified> after > parse_options() returns, according to the value in > checkout_stage. > > and then this can follow the "opt->value points at the variable that > is affected" pattern. Yeah, I think that would work, with one extra bit: (4) complain when (!to_tempfile && checkout_stage == CHECKOUT_ALL) I do think it would be better to fix separately, but maybe if I'm re-rolling I can do it as an early patch in the series (it is not much different than the "xopts" fix in scope). > > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset) > > +static int parse_refmap_arg(const struct option *opt UNUSED, > > + const char *arg, int unset) > > { > > BUG_ON_OPT_NEG(unset); > > Can't this just point opt->value at the global &refmap? Obviously > not a huge deal, as we could have taken the "annotate as UNUSED" > approach for all the functions in [3/8]. Hmm, yeah. I think I looked at the abstract refspec_append() here and assumed that it might be touching other variables. But it's not. It's operating purely on the &refspec we pass it (and even though it uses ALLOC_GROW, the "nr" and "alloc" are both contained in the struct). So yeah, it really should have been converted in patch 3. I think that is probably worth a re-roll. -Peff
On Thu, Aug 31, 2023 at 01:50:18PM -0400, Jeff King wrote: > > > -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset) > > > +static int parse_refmap_arg(const struct option *opt UNUSED, > > > + const char *arg, int unset) > > > { > > > BUG_ON_OPT_NEG(unset); > > > > Can't this just point opt->value at the global &refmap? Obviously > > not a huge deal, as we could have taken the "annotate as UNUSED" > > approach for all the functions in [3/8]. > > Hmm, yeah. I think I looked at the abstract refspec_append() here and > assumed that it might be touching other variables. But it's not. It's > operating purely on the &refspec we pass it (and even though it uses > ALLOC_GROW, the "nr" and "alloc" are both contained in the struct). So > yeah, it really should have been converted in patch 3. Oh, btw, there are two other cases in this patch that _could_ be converted but I left alone. In log.c, we have a to_callback() and a cc_callback(), both of which take a single string list. But there is also header_callback(), which resets both of those when it sees --no-add-header. So I left them alone as a set. However, it occurs to me that to_callback() and cc_callback() can just be OPT_STRING_LIST these days. And that is probably a worthwhile cleanup to do. -Peff
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c index f62f13f2b5..95b3717dd1 100644 --- a/builtin/checkout-index.c +++ b/builtin/checkout-index.c @@ -190,7 +190,7 @@ static const char * const builtin_checkout_index_usage[] = { NULL }; -static int option_parse_stage(const struct option *opt, +static int option_parse_stage(const struct option *opt UNUSED, const char *arg, int unset) { BUG_ON_OPT_NEG(unset); diff --git a/builtin/fetch.c b/builtin/fetch.c index 8f93529505..cd2afe33e5 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -168,7 +168,8 @@ static int git_fetch_config(const char *k, const char *v, return git_default_config(k, v, ctx, cb); } -static int parse_refmap_arg(const struct option *opt, const char *arg, int unset) +static int parse_refmap_arg(const struct option *opt UNUSED, + const char *arg, int unset) { BUG_ON_OPT_NEG(unset); 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 db3a88bfe9..87e29c4171 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); @@ -1567,7 +1569,8 @@ static int header_callback(const struct option *opt, const char *arg, int unset) return 0; } -static int to_callback(const struct option *opt, const char *arg, int unset) +static int to_callback(const struct option *opt UNUSED, const char *arg, + int unset) { if (unset) string_list_clear(&extra_to, 0); @@ -1576,7 +1579,8 @@ static int to_callback(const struct option *opt, const char *arg, int unset) return 0; } -static int cc_callback(const struct option *opt, const char *arg, int unset) +static int cc_callback(const struct option *opt UNUSED, const char *arg, + int unset) { if (unset) string_list_clear(&extra_cc, 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/checkout-index.c | 2 +- builtin/fetch.c | 3 ++- builtin/gc.c | 2 +- builtin/log.c | 16 ++++++++++------ builtin/merge.c | 2 +- builtin/pack-objects.c | 6 +++--- builtin/read-tree.c | 2 +- builtin/update-index.c | 4 ++-- 8 files changed, 21 insertions(+), 16 deletions(-)