| Message ID | patch-v3-03.10-828e8b96574-20211008T190536Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 352e761388b5fa41bf40e7c04edf3cb07d888d94 |
| Headers | show |
| Series | fix bug, use more enums | expand |
On Fri, Oct 08, 2021 at 09:07:39PM +0200, Ævar Arnfjörð Bjarmason wrote: > Use the "enum parse_opt_result" instead of an "int flags" as the > return value of the applicable functions in parse-options.c. > > This will help catch future bugs, such as the missing "case" arms in > the two existing users of the API in "blame.c" and "shortlog.c". A > third caller in 309be813c9b (update-index: migrate to parse-options > API, 2010-12-01) was already checking for these. > > As can be seen when trying to sort through the deluge of warnings > produced when compiling this with CC=g++ (mostly unrelated to this > change) we're not consistently using "enum parse_opt_result" even now, > i.e. we'll return error() and "return 0;". See f41179f16ba > (parse-options: avoid magic return codes, 2019-01-27) for a commit > which started changing some of that. > > I'm not doing any more of that exhaustive migration here, and it's > probably not worthwhile past the point of being able to check "enum > parse_opt_result" in switch(). > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > diff --git a/parse-options.c b/parse-options.c > index 9c8ba963400..f718242096c 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -699,13 +699,14 @@ static void free_preprocessed_options(struct option *options) > free(options); > } > > -static int usage_with_options_internal(struct parse_opt_ctx_t *, > - const char * const *, > - const struct option *, int, int); > - > -int parse_options_step(struct parse_opt_ctx_t *ctx, > - const struct option *options, > - const char * const usagestr[]) > +static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *, > + const char * const *, > + const struct option *, > + int, int); > + > +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, > + const struct option *options, > + const char * const usagestr[]) > { > int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP); > > @@ -839,10 +840,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx) > return ctx->cpidx + ctx->argc; > } > > -int parse_options(int argc, const char **argv, const char *prefix, > - const struct option *options, > - const char * const usagestr[], > - enum parse_opt_flags flags) > +enum parse_opt_result parse_options(int argc, const char **argv, The return type of this function should have been left unchanged, because it contains only one return statement: return parse_options_end(&ctx); and parse_options_end() does return an int.
On Sat, Nov 06 2021, SZEDER Gábor wrote: > On Fri, Oct 08, 2021 at 09:07:39PM +0200, Ævar Arnfjörð Bjarmason wrote: >> Use the "enum parse_opt_result" instead of an "int flags" as the >> return value of the applicable functions in parse-options.c. >> >> This will help catch future bugs, such as the missing "case" arms in >> the two existing users of the API in "blame.c" and "shortlog.c". A >> third caller in 309be813c9b (update-index: migrate to parse-options >> API, 2010-12-01) was already checking for these. >> >> As can be seen when trying to sort through the deluge of warnings >> produced when compiling this with CC=g++ (mostly unrelated to this >> change) we're not consistently using "enum parse_opt_result" even now, >> i.e. we'll return error() and "return 0;". See f41179f16ba >> (parse-options: avoid magic return codes, 2019-01-27) for a commit >> which started changing some of that. >> >> I'm not doing any more of that exhaustive migration here, and it's >> probably not worthwhile past the point of being able to check "enum >> parse_opt_result" in switch(). >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- > >> diff --git a/parse-options.c b/parse-options.c >> index 9c8ba963400..f718242096c 100644 >> --- a/parse-options.c >> +++ b/parse-options.c >> @@ -699,13 +699,14 @@ static void free_preprocessed_options(struct option *options) >> free(options); >> } >> >> -static int usage_with_options_internal(struct parse_opt_ctx_t *, >> - const char * const *, >> - const struct option *, int, int); >> - >> -int parse_options_step(struct parse_opt_ctx_t *ctx, >> - const struct option *options, >> - const char * const usagestr[]) >> +static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *, >> + const char * const *, >> + const struct option *, >> + int, int); >> + >> +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, >> + const struct option *options, >> + const char * const usagestr[]) >> { >> int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP); >> >> @@ -839,10 +840,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx) >> return ctx->cpidx + ctx->argc; >> } >> >> -int parse_options(int argc, const char **argv, const char *prefix, >> - const struct option *options, >> - const char * const usagestr[], >> - enum parse_opt_flags flags) >> +enum parse_opt_result parse_options(int argc, const char **argv, > > The return type of this function should have been left unchanged, > because it contains only one return statement: > > return parse_options_end(&ctx); > > and parse_options_end() does return an int. Indeed. I'll submit a fix for that sooner than later. I think I got that and *_step() mixed up, parse_options() just returns "here's what we've got left in argc". I think due to C's happy-go-lucky enum-as-int semantics I'll probably leave it until post-2.34, i.e. I don't think it can/will break anything at runtime, but should be fixed for sure. Thanks for spotting!
diff --git a/builtin/blame.c b/builtin/blame.c index 641523ff9af..9273fb222d5 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -917,6 +917,9 @@ int cmd_blame(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0); for (;;) { switch (parse_options_step(&ctx, options, blame_opt_usage)) { + case PARSE_OPT_NON_OPTION: + case PARSE_OPT_UNKNOWN: + break; case PARSE_OPT_HELP: case PARSE_OPT_ERROR: exit(129); diff --git a/builtin/shortlog.c b/builtin/shortlog.c index 3e7ab1ca821..e7f7af5de3f 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -374,6 +374,9 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) for (;;) { switch (parse_options_step(&ctx, options, shortlog_usage)) { + case PARSE_OPT_NON_OPTION: + case PARSE_OPT_UNKNOWN: + break; case PARSE_OPT_HELP: case PARSE_OPT_ERROR: exit(129); diff --git a/parse-options.c b/parse-options.c index 9c8ba963400..f718242096c 100644 --- a/parse-options.c +++ b/parse-options.c @@ -699,13 +699,14 @@ static void free_preprocessed_options(struct option *options) free(options); } -static int usage_with_options_internal(struct parse_opt_ctx_t *, - const char * const *, - const struct option *, int, int); - -int parse_options_step(struct parse_opt_ctx_t *ctx, - const struct option *options, - const char * const usagestr[]) +static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *, + const char * const *, + const struct option *, + int, int); + +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, + const struct option *options, + const char * const usagestr[]) { int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP); @@ -839,10 +840,11 @@ int parse_options_end(struct parse_opt_ctx_t *ctx) return ctx->cpidx + ctx->argc; } -int parse_options(int argc, const char **argv, const char *prefix, - const struct option *options, - const char * const usagestr[], - enum parse_opt_flags flags) +enum parse_opt_result parse_options(int argc, const char **argv, + const char *prefix, + const struct option *options, + const char * const usagestr[], + enum parse_opt_flags flags) { struct parse_opt_ctx_t ctx; struct option *real_options; @@ -900,9 +902,10 @@ static int usage_argh(const struct option *opts, FILE *outfile) #define USAGE_OPTS_WIDTH 24 #define USAGE_GAP 2 -static int usage_with_options_internal(struct parse_opt_ctx_t *ctx, - const char * const *usagestr, - const struct option *opts, int full, int err) +static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx, + const char * const *usagestr, + const struct option *opts, + int full, int err) { FILE *outfile = err ? stderr : stdout; int need_newline; diff --git a/parse-options.h b/parse-options.h index 2e8798d8744..a1c7c86ad30 100644 --- a/parse-options.h +++ b/parse-options.h @@ -211,10 +211,11 @@ struct option { * untouched and parse_options() returns the number of options * processed. */ -int parse_options(int argc, const char **argv, const char *prefix, - const struct option *options, - const char * const usagestr[], - enum parse_opt_flags flags); +enum parse_opt_result parse_options(int argc, const char **argv, + const char *prefix, + const struct option *options, + const char * const usagestr[], + enum parse_opt_flags flags); NORETURN void usage_with_options(const char * const *usagestr, const struct option *options); @@ -274,9 +275,9 @@ void parse_options_start(struct parse_opt_ctx_t *ctx, const struct option *options, enum parse_opt_flags flags); -int parse_options_step(struct parse_opt_ctx_t *ctx, - const struct option *options, - const char * const usagestr[]); +enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx, + const struct option *options, + const char * const usagestr[]); int parse_options_end(struct parse_opt_ctx_t *ctx);
Use the "enum parse_opt_result" instead of an "int flags" as the return value of the applicable functions in parse-options.c. This will help catch future bugs, such as the missing "case" arms in the two existing users of the API in "blame.c" and "shortlog.c". A third caller in 309be813c9b (update-index: migrate to parse-options API, 2010-12-01) was already checking for these. As can be seen when trying to sort through the deluge of warnings produced when compiling this with CC=g++ (mostly unrelated to this change) we're not consistently using "enum parse_opt_result" even now, i.e. we'll return error() and "return 0;". See f41179f16ba (parse-options: avoid magic return codes, 2019-01-27) for a commit which started changing some of that. I'm not doing any more of that exhaustive migration here, and it's probably not worthwhile past the point of being able to check "enum parse_opt_result" in switch(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/blame.c | 3 +++ builtin/shortlog.c | 3 +++ parse-options.c | 31 +++++++++++++++++-------------- parse-options.h | 15 ++++++++------- 4 files changed, 31 insertions(+), 21 deletions(-)