| Message ID | patch-04.10-624a19000e1-20210928T130905Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix bug, use existing enums | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Change code in get_value(), parse_options_check() etc. to do away with > the "default" case in favor of exhaustively checking the relevant > fields. I am not sure if this is a good idea in the bigger picture. If we know that unlisted cases should not happen, having "default: BUG()" without explicitly listing them is just as expressive as the result of this patch with much shorter code. When a new enum member is added without adding corresponding processing of the new value, either way it will be caught as a BUG(). Removing "default: BUG()" does allow you to catch such an error at compilation time, and keeping it may prevent you from doing so, but in practice, you'd be adding test coverage for the new case, which means that, even if your compiler is not cooperating, your test suite addition will hit "default: BUG();" in such a case. So ... > - default: > + /* special types */ > + case OPTION_END: > + case OPTION_GROUP: > + case OPTION_NUMBER: > + case OPTION_ALIAS: > BUG("opt->type %d should not happen", opt->type); > } > + return -1; /* gcc 10.2.1-6's -Werror=return-type */ > }
On Tue, Sep 28 2021, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Change code in get_value(), parse_options_check() etc. to do away with >> the "default" case in favor of exhaustively checking the relevant >> fields. > > I am not sure if this is a good idea in the bigger picture. > > If we know that unlisted cases should not happen, having "default: > BUG()" without explicitly listing them is just as expressive as the > result of this patch with much shorter code. > > When a new enum member is added without adding corresponding > processing of the new value, either way it will be caught as a > BUG(). Removing "default: BUG()" does allow you to catch such an > error at compilation time, and keeping it may prevent you from doing > so, but in practice, you'd be adding test coverage for the new case, > which means that, even if your compiler is not cooperating, your > test suite addition will hit "default: BUG();" in such a case. Yes we'll catch them since these are loops over all options. But that assumes that: 1) You have a test that's stressing the relevant entry point 2) That you run all your tests, e.g. one of these entry points is the "git completion" one I think listing the remaining enum arms is a small price to pay for having that all moved to compile-time. >> - default: >> + /* special types */ >> + case OPTION_END: >> + case OPTION_GROUP: >> + case OPTION_NUMBER: >> + case OPTION_ALIAS: >> BUG("opt->type %d should not happen", opt->type); >> } >> + return -1; /* gcc 10.2.1-6's -Werror=return-type */ >> }
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > I think listing the remaining enum arms is a small price to pay for > having that all moved to compile-time. I can sympathize with that point of view, since I used to think the same way, but I am not yet convinced. An example like this from your postimage, which doubles the size of a switch statement with empty case arms, demonstrates that it is not a "small" price. Admittedly, the original switch statement is particularly bad, though ;-) switch (opts->type) { case OPTION_STRING: case OPTION_FILENAME: case OPTION_INTEGER: case OPTION_MAGNITUDE: case OPTION_CALLBACK: case OPTION_BIT: case OPTION_NEGBIT: case OPTION_COUNTUP: case OPTION_SET_INT: has_unset_form = 1; break; - default: + /* special types */ + case OPTION_END: + case OPTION_GROUP: + case OPTION_NUMBER: + case OPTION_ALIAS: + /* options with no arguments */ + case OPTION_BITOP: + /* options with arguments (usually) */ + case OPTION_LOWLEVEL_CALLBACK: break; }
diff --git a/parse-options.c b/parse-options.c index 799cd884f2b..733cbfa8821 100644 --- a/parse-options.c +++ b/parse-options.c @@ -219,9 +219,14 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p, optname(opt, flags)); return 0; - default: + /* special types */ + case OPTION_END: + case OPTION_GROUP: + case OPTION_NUMBER: + case OPTION_ALIAS: BUG("opt->type %d should not happen", opt->type); } + return -1; /* gcc 10.2.1-6's -Werror=return-type */ } static enum parse_opt_result parse_short_opt(struct parse_opt_ctx_t *p, @@ -468,8 +473,15 @@ static void parse_options_check(const struct option *opts) BUG("OPT_ALIAS() should not remain at this point. " "Are you using parse_options_step() directly?\n" "That case is not supported yet."); - default: - ; /* ok. (usually accepts an argument) */ + + case OPTION_BITOP: + case OPTION_END: + case OPTION_FILENAME: + case OPTION_GROUP: + case OPTION_INTEGER: + case OPTION_MAGNITUDE: + case OPTION_STRING: + break; } if (opts->argh && strcspn(opts->argh, " _") != strlen(opts->argh)) @@ -543,7 +555,15 @@ static void show_negated_gitcomp(const struct option *opts, int show_all, case OPTION_SET_INT: has_unset_form = 1; break; - default: + /* special types */ + case OPTION_END: + case OPTION_GROUP: + case OPTION_NUMBER: + case OPTION_ALIAS: + /* options with no arguments */ + case OPTION_BITOP: + /* options with arguments (usually) */ + case OPTION_LOWLEVEL_CALLBACK: break; } if (!has_unset_form) @@ -593,7 +613,20 @@ static int show_gitcomp(const struct option *opts, int show_all) break; suffix = "="; break; - default: + /* special types */ + case OPTION_END: + case OPTION_NUMBER: + case OPTION_ALIAS: + + /* options with no arguments */ + case OPTION_BIT: + case OPTION_NEGBIT: + case OPTION_BITOP: + case OPTION_COUNTUP: + case OPTION_SET_INT: + + /* options with arguments (usually) */ + case OPTION_LOWLEVEL_CALLBACK: break; } if (opts->flags & PARSE_OPT_COMP_ARG) diff --git a/parse-options.h b/parse-options.h index d931300f4d6..a1c7c86ad30 100644 --- a/parse-options.h +++ b/parse-options.h @@ -264,7 +264,7 @@ struct parse_opt_ctx_t { const char **out; int argc, cpidx, total; const char *opt; - int flags; + enum parse_opt_flags flags; const char *prefix; const char **alias_groups; /* must be in groups of 3 elements! */ struct option *updated_options;
Change code in get_value(), parse_options_check() etc. to do away with the "default" case in favor of exhaustively checking the relevant fields. The added "return -1" is needed for the GCC version commented on inline, my local clang 11.0.1-2 does not require it. Let's add it for now to appease GCC. The added "special types" etc. comments correspond to the relevant comments and ordering on the "enum parse_opt_type". Let's try to keep the same order and commentary as there where possible for clarity. This doesn't reach that end-state, and due to the different handling of options it's probably not worth it to get there, but let's match its ordering where it's easy to do so. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- parse-options.c | 43 ++++++++++++++++++++++++++++++++++++++----- parse-options.h | 2 +- 2 files changed, 39 insertions(+), 6 deletions(-)