| Message ID | 20181209102724.8707-1-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | parse-options: fix SunCC compiler warning | expand |
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > The compiler reports this because show_gitcomp() never actually > returns a value: > > "parse-options.c", line 520: warning: Function has no return > statement : show_gitcomp > > The function calls exit() and will never return. Update and mark it > NORETURN. Yuck. This should do for now, but I am not impressed by the choice to hook show_gitcomp() call into parse_options_step(), which forces such an abnormal exit deeper in the callchain [*1*]. For readers (not compilers), it would help to have a comment at the caller that says that show_gitcomp() never returns and exits. side note #1. Perhaps parse_options_start() would have been a better place, instead of parse_options_step() that is repeatedly called in a loop and itself has a loop. ANd worse yet, the check is done inside the loop even though the call is to be made only when the --git-completion-helper option is given as the sole request. Thanks. > Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > --- > parse-options.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/parse-options.c b/parse-options.c > index 3b874a83a0..6577e52f63 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -474,8 +474,8 @@ static void show_negated_gitcomp(const struct option *opts, int nr_noopts) > } > } > > -static int show_gitcomp(struct parse_opt_ctx_t *ctx, > - const struct option *opts) > +static void NORETURN show_gitcomp(struct parse_opt_ctx_t *ctx, > + const struct option *opts) > { > const struct option *original_opts = opts; > int nr_noopts = 0; > @@ -550,7 +550,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx, > > /* lone --git-completion-helper is asked by git-completion.bash */ > if (ctx->total == 1 && !strcmp(arg + 1, "-git-completion-helper")) > - return show_gitcomp(ctx, options); > + show_gitcomp(ctx, options); > > if (arg[1] != '-') { > ctx->opt = arg + 1;
On Mon, Dec 10, 2018 at 03:36:54PM +0900, Junio C Hamano wrote: > Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > > > The compiler reports this because show_gitcomp() never actually > > returns a value: > > > > "parse-options.c", line 520: warning: Function has no return > > statement : show_gitcomp > > > > The function calls exit() and will never return. Update and mark it > > NORETURN. > > Yuck. This should do for now, but I am not impressed by the choice > to hook show_gitcomp() call into parse_options_step(), which forces > such an abnormal exit deeper in the callchain [*1*]. For readers > (not compilers), it would help to have a comment at the caller that > says that show_gitcomp() never returns and exits. > > side note #1. Perhaps parse_options_start() would have been > a better place, instead of parse_options_step() that is > repeatedly called in a loop and itself has a loop. ANd > worse yet, the check is done inside the loop even though the > call is to be made only when the --git-completion-helper > option is given as the sole request. The reason it's in parse_options_step() is because -h is also handled in there. Although -h does not bury exit() deep in the call chain. So how about this as a replacement? -- 8< -- diff --git a/parse-options.c b/parse-options.c index 3b874a83a0..6932eaff61 100644 --- a/parse-options.c +++ b/parse-options.c @@ -516,7 +516,7 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx, show_negated_gitcomp(original_opts, -1); show_negated_gitcomp(original_opts, nr_noopts); fputc('\n', stdout); - exit(0); + return PARSE_OPT_COMPLETE; } static int usage_with_options_internal(struct parse_opt_ctx_t *, @@ -638,6 +638,8 @@ int parse_options(int argc, const char **argv, const char *prefix, case PARSE_OPT_HELP: case PARSE_OPT_ERROR: exit(129); + case PARSE_OPT_COMPLETE: + exit(0); case PARSE_OPT_NON_OPTION: case PARSE_OPT_DONE: break; diff --git a/parse-options.h b/parse-options.h index 6c4fe2016d..a650a7d220 100644 --- a/parse-options.h +++ b/parse-options.h @@ -208,6 +208,7 @@ extern int opterror(const struct option *opt, const char *reason, int flags); /*----- incremental advanced APIs -----*/ enum { + PARSE_OPT_COMPLETE = -2, PARSE_OPT_HELP = -1, PARSE_OPT_DONE, PARSE_OPT_NON_OPTION, -- 8< --
Duy Nguyen <pclouds@gmail.com> writes: > The reason it's in parse_options_step() is because -h is also handled > in there. Although -h does not bury exit() deep in the call chain. So > how about this as a replacement? So just like -h returns PARSE_OPT_HELP and causes the calling parse_options() to exit(129), the new _COMPLETE can be used to trigger an early & successful return? We'd also have to cover builtin/{blame,shortlog,update-index}.c where they switch() on the return value from parse_options_step(), but other than that, it probably is a better approach. The users of parse_options_step() that avoid parse_options() want to handle the various "we stopped by hitting a non-option" cases in their own ways, so treating this special action the same way as -h is treated would be sensible. We _might_ want to standardize some of the case arms in these custom switch statements that appear in these three built-in command implementations, but that can be done more easily if this step is done like you showed below, I think. > -- 8< -- > diff --git a/parse-options.c b/parse-options.c > index 3b874a83a0..6932eaff61 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -516,7 +516,7 @@ static int show_gitcomp(struct parse_opt_ctx_t *ctx, > show_negated_gitcomp(original_opts, -1); > show_negated_gitcomp(original_opts, nr_noopts); > fputc('\n', stdout); > - exit(0); > + return PARSE_OPT_COMPLETE; > } > > static int usage_with_options_internal(struct parse_opt_ctx_t *, > @@ -638,6 +638,8 @@ int parse_options(int argc, const char **argv, const char *prefix, > case PARSE_OPT_HELP: > case PARSE_OPT_ERROR: > exit(129); > + case PARSE_OPT_COMPLETE: > + exit(0); > case PARSE_OPT_NON_OPTION: > case PARSE_OPT_DONE: > break; > diff --git a/parse-options.h b/parse-options.h > index 6c4fe2016d..a650a7d220 100644 > --- a/parse-options.h > +++ b/parse-options.h > @@ -208,6 +208,7 @@ extern int opterror(const struct option *opt, const char *reason, int flags); > /*----- incremental advanced APIs -----*/ > > enum { > + PARSE_OPT_COMPLETE = -2, > PARSE_OPT_HELP = -1, > PARSE_OPT_DONE, > PARSE_OPT_NON_OPTION, > -- 8< --
On Tue, Dec 11, 2018 at 3:13 AM Junio C Hamano <gitster@pobox.com> wrote: > > Duy Nguyen <pclouds@gmail.com> writes: > > > The reason it's in parse_options_step() is because -h is also handled > > in there. Although -h does not bury exit() deep in the call chain. So > > how about this as a replacement? > > So just like -h returns PARSE_OPT_HELP and causes the calling > parse_options() to exit(129), the new _COMPLETE can be used to > trigger an early & successful return? > > We'd also have to cover builtin/{blame,shortlog,update-index}.c > where they switch() on the return value from parse_options_step(), > but other than that, it probably is a better approach. The users of > parse_options_step() that avoid parse_options() want to handle the > various "we stopped by hitting a non-option" cases in their own > ways, so treating this special action the same way as -h is treated > would be sensible. Yeah, I saw that shortly after sending the patch. > We _might_ want to standardize some of the case > arms in these custom switch statements that appear in these three > built-in command implementations, but that can be done more easily > if this step is done like you showed below, I think. I have a better plan: stop exposing parse-options loop to outside. What all these commands need is the ability to deal with unknown options (or non-options in update-index case). They could register callbacks to deal with those and keep calling parse_options() instead. I'm converting rev-list to use parse_options() and as an intermediate step, this callback idea should work well. The end game though is a single parse_options() call that covers everything in, say, git-log. But we will see.
Duy Nguyen <pclouds@gmail.com> writes: > I have a better plan: stop exposing parse-options loop to outside. > What all these commands need is the ability to deal with unknown > options (or non-options in update-index case). They could register > callbacks to deal with those and keep calling parse_options() instead. > I'm converting rev-list to use parse_options() and as an intermediate > step, this callback idea should work well. The end game though is a > single parse_options() call that covers everything in, say, git-log. > But we will see. Sounds like a good plan. Thanks.
diff --git a/parse-options.c b/parse-options.c index 3b874a83a0..6577e52f63 100644 --- a/parse-options.c +++ b/parse-options.c @@ -474,8 +474,8 @@ static void show_negated_gitcomp(const struct option *opts, int nr_noopts) } } -static int show_gitcomp(struct parse_opt_ctx_t *ctx, - const struct option *opts) +static void NORETURN show_gitcomp(struct parse_opt_ctx_t *ctx, + const struct option *opts) { const struct option *original_opts = opts; int nr_noopts = 0; @@ -550,7 +550,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx, /* lone --git-completion-helper is asked by git-completion.bash */ if (ctx->total == 1 && !strcmp(arg + 1, "-git-completion-helper")) - return show_gitcomp(ctx, options); + show_gitcomp(ctx, options); if (arg[1] != '-') { ctx->opt = arg + 1;
The compiler reports this because show_gitcomp() never actually returns a value: "parse-options.c", line 520: warning: Function has no return statement : show_gitcomp The function calls exit() and will never return. Update and mark it NORETURN. Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- parse-options.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)