| Message ID | 20200315135300.GA32701@ldh-macbook.local |
|---|---|
| State | New |
| Headers | show |
| Series | driver: Improve the generated help text for alias options with arguments | expand |
Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > Hello- > > Currently, if an option is both undocumented and an alias for a second option, > the help text generated by gcc --help directs the reader to use the other > option instead. This makes sense for deprecated options, but it seems this > pattern is also used for another case, namely when an option exists so as to > pass a default argument to another option. For instance this one, from > common.opt: > > ------------- > fdiagnostics-color > Common Alias(fdiagnostics-color=,always,never) > ; > > fdiagnostics-color= > Driver Common Joined RejectNegative Var(flag_diagnostics_show_color) Enum(diagnostic_color_rule) Init(DIAGNOSTICS_COLOR_NO) > -fdiagnostics-color=[never|always|auto] Colorize diagnostics. > ------------- > > This is nice because it means you can say -fdiagnostics-color as a shorthand > for -fdiagnostics-color=always, or -fno-diagnostics-color as a shorthand for > -fdiagnostics-color=never. However, the generated help text does not describe > it this way: > > ------------- > $ gcc --help=common | grep fdiagnostics-color > -fdiagnostics-color Same as -fdiagnostics-color=. Use the latter option instead. > -fdiagnostics-color=[never|always|auto] Colorize diagnostics. > ------------- > > Perhaps I am wrong and the non-argument usage is indeed meant to be deprecated, > but it feels more like it was intended as a convenience and could be documented > as such. What actually prompted this patch is that I am adding a new option for > GCC 11 with these never/always/auto semantics and I am a bit confused whether I > am supposed to add the aliased version or not. Feels like it's nice to add it, > but then the --help output says the opposite... > > Anyway, the attached patch would change the help output to the following... If > that seems to be an improvement and closer to the intended behavior, please let > me know. Thanks! > > ------------- > -fdiagnostics-color Same as -fdiagnostics-color=always (or, in negated form, -fdiagnostics-color=never). > -fdiagnostics-color=[never|always|auto] Colorize diagnostics. > ------------- > > FWIW there are three other options currently affected by this change > (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for > -Wimplicit-fallthrough I think is particularly helpful: > > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. > becomes > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). I also see: - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. ... --imacros Same as -imacros. Use the latter option instead. --imacros= Same as -imacros. Use the latter option instead. --include Same as -include. Use the latter option instead. - --include-barrier Same as -I. Use the latter option instead. + --include-barrier Same as -I-. --include-directory Same as -I. Use the latter option instead. --include-directory-after Same as -idirafter. Use the latter option instead. --include-directory-after= Same as -idirafter. Use the latter option instead. ... - -Wnormalized Same as -Wnormalized=. Use the latter option instead. + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. I agree all of these look like improvements, especially the --include-barrier one. But I think the include ones also show that the "Use the latter option instead." decision is independent of whether the option is defined to be an alias. FWIW, there's also: Wmissing-format-attribute C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) ; which still ends up as: -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. Not really my area though, so I don't have any specific suggestion about how to separate the cases. Thanks, Richard
On Mon, Mar 16, 2020 at 06:11:08PM +0000, Richard Sandiford wrote: > Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: ... > > FWIW there are three other options currently affected by this change > > (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for > > -Wimplicit-fallthrough I think is particularly helpful: > > > > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. > > becomes > > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). > > I also see: > > - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. > + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). > -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. > ... > --imacros Same as -imacros. Use the latter option instead. > --imacros= Same as -imacros. Use the latter option instead. > --include Same as -include. Use the latter option instead. > - --include-barrier Same as -I. Use the latter option instead. > + --include-barrier Same as -I-. > --include-directory Same as -I. Use the latter option instead. > --include-directory-after Same as -idirafter. Use the latter option instead. > --include-directory-after= Same as -idirafter. Use the latter option instead. > ... > - -Wnormalized Same as -Wnormalized=. Use the latter option instead. > + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). > -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. > > I agree all of these look like improvements, especially the > --include-barrier one. But I think the include ones also show > that the "Use the latter option instead." decision is independent > of whether the option is defined to be an alias. > > FWIW, there's also: > > Wmissing-format-attribute > C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) > ; > > which still ends up as: > > -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. > > Not really my area though, so I don't have any specific suggestion > about how to separate the cases. > Right, sorry, in my first email I only mentioned the options output by --help=common, but there were a few more as well. Thanks very much for trying it out and for the feedback. The rule I implemented was to change the help output for all alias options with no documentation if they also specify the extra 2nd option (or 2nd and 3rd options) to the Alias directive. For example, -include-barrier is like this: -include-barrier C ObjC C++ ObjC++ Alias(I, -) It serves to provide the argument '-' to the option '-I', so it is eligible for the new text. The others are like this one: -include-directory-after C ObjC C++ ObjC++ Separate Alias(idirafter) MissingArgError(missing path after %qs) Since that one doesn't pass the extra args to Alias, I interpreted it to mean this is the case for which the "Use the latter option" directive was intended to apply. (-idirafter has been designated as preferable to -include-directory-after). Regarding -Wmissing-format-attribute, that is an interesting case, it's the only instance in any *.opt that has an = sign in the Alias target. If I understand correctly, this one can't use the 3-argument form of Alias() because -Wno-missing-format-attribute is actually an alias for -Wno-suggest-attribute=format, rather than -Wsuggest-attribute=<something>. I think that one could be left as is or else my patch would need to check for an = sign in the alias target and handle this case as well, but that may lead to other surprises in the future. Thanks! -Lewis
On Mon, Mar 16, 2020 at 6:11 PM Lewis Hyatt <lhyatt@gmail.com> wrote: > Regarding -Wmissing-format-attribute, that is an interesting case, it's the only > instance in any *.opt that has an = sign in the Alias target. If I understand > correctly, this one can't use the 3-argument form of Alias() because > -Wno-missing-format-attribute is actually an alias for > -Wno-suggest-attribute=format, rather than -Wsuggest-attribute=<something>. I > think that one could be left as is or else my patch would need to check for an = > sign in the alias target and handle this case as well, but that may lead to > other surprises in the future. Sorry, please disregard the above paragraph, that was my mistake... Those kinds of aliases with an = are used frequently. I guess the current behavior is probably best for them. -Lewis
Lewis Hyatt <lhyatt@gmail.com> writes: > On Mon, Mar 16, 2020 at 06:11:08PM +0000, Richard Sandiford wrote: >> Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > ... >> > FWIW there are three other options currently affected by this change >> > (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for >> > -Wimplicit-fallthrough I think is particularly helpful: >> > >> > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. >> > becomes >> > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). >> >> I also see: >> >> - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. >> + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). >> -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. >> ... >> --imacros Same as -imacros. Use the latter option instead. >> --imacros= Same as -imacros. Use the latter option instead. >> --include Same as -include. Use the latter option instead. >> - --include-barrier Same as -I. Use the latter option instead. >> + --include-barrier Same as -I-. >> --include-directory Same as -I. Use the latter option instead. >> --include-directory-after Same as -idirafter. Use the latter option instead. >> --include-directory-after= Same as -idirafter. Use the latter option instead. >> ... >> - -Wnormalized Same as -Wnormalized=. Use the latter option instead. >> + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). >> -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. >> >> I agree all of these look like improvements, especially the >> --include-barrier one. But I think the include ones also show >> that the "Use the latter option instead." decision is independent >> of whether the option is defined to be an alias. Gah, I meant an Alias() with an argument here. >> >> FWIW, there's also: >> >> Wmissing-format-attribute >> C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) >> ; >> >> which still ends up as: >> >> -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. >> >> Not really my area though, so I don't have any specific suggestion >> about how to separate the cases. >> > > Right, sorry, in my first email I only mentioned the options output by > --help=common, but there were a few more as well. Thanks very much for trying > it out and for the feedback. > > The rule I implemented was to change the help output for all alias options > with no documentation if they also specify the extra 2nd option (or 2nd and > 3rd options) to the Alias directive. For example, -include-barrier is like this: > > -include-barrier C ObjC C++ ObjC++ Alias(I, -) > > It serves to provide the argument '-' to the option '-I', so it is eligible for > the new text. The others are like this one: > > -include-directory-after C ObjC C++ ObjC++ Separate Alias(idirafter) MissingArgError(missing path after %qs) > > Since that one doesn't pass the extra args to Alias, I interpreted it to > mean this is the case for which the "Use the latter option" directive was > intended to apply. (-idirafter has been designated as preferable to > -include-directory-after). Yeah, I get why you did it like this. It's just that the end effect is to make --include-barrier seem less disparaged than the other --include-* options, but I'm not sure there's supposed to be any difference between them in that respect. Perhaps we should drop the "Use the latter option instead." thing altogether for aliases. I'm not sure that it really helps, and this thread shows that adding it automatically can lead to some odd corner cases. In practice we shouldn't remove any of these aliases unless we're also removing the option that they're an alias of. And if we do that, the options should go through the usual deprecation cycle, just like options without aliases. If there are specific options that we want to steer users away from without deprecation, then we should probably have a specific tag for that. Thanks, Richard
On Tue, Mar 17, 2020 at 11:52:13AM +0000, Richard Sandiford wrote: > Lewis Hyatt <lhyatt@gmail.com> writes: > > On Mon, Mar 16, 2020 at 06:11:08PM +0000, Richard Sandiford wrote: > >> Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > > ... > >> > FWIW there are three other options currently affected by this change > >> > (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for > >> > -Wimplicit-fallthrough I think is particularly helpful: > >> > > >> > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. > >> > becomes > >> > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). > >> > >> I also see: > >> > >> - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. > >> + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). > >> -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. > >> ... > >> --imacros Same as -imacros. Use the latter option instead. > >> --imacros= Same as -imacros. Use the latter option instead. > >> --include Same as -include. Use the latter option instead. > >> - --include-barrier Same as -I. Use the latter option instead. > >> + --include-barrier Same as -I-. > >> --include-directory Same as -I. Use the latter option instead. > >> --include-directory-after Same as -idirafter. Use the latter option instead. > >> --include-directory-after= Same as -idirafter. Use the latter option instead. > >> ... > >> - -Wnormalized Same as -Wnormalized=. Use the latter option instead. > >> + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). > >> -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. > >> > >> I agree all of these look like improvements, especially the > >> --include-barrier one. But I think the include ones also show > >> that the "Use the latter option instead." decision is independent > >> of whether the option is defined to be an alias. > > Gah, I meant an Alias() with an argument here. > > >> > >> FWIW, there's also: > >> > >> Wmissing-format-attribute > >> C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) > >> ; > >> > >> which still ends up as: > >> > >> -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. > >> > >> Not really my area though, so I don't have any specific suggestion > >> about how to separate the cases. > >> > > > > Right, sorry, in my first email I only mentioned the options output by > > --help=common, but there were a few more as well. Thanks very much for trying > > it out and for the feedback. > > > > The rule I implemented was to change the help output for all alias options > > with no documentation if they also specify the extra 2nd option (or 2nd and > > 3rd options) to the Alias directive. For example, -include-barrier is like this: > > > > -include-barrier C ObjC C++ ObjC++ Alias(I, -) > > > > It serves to provide the argument '-' to the option '-I', so it is eligible for > > the new text. The others are like this one: > > > > -include-directory-after C ObjC C++ ObjC++ Separate Alias(idirafter) MissingArgError(missing path after %qs) > > > > Since that one doesn't pass the extra args to Alias, I interpreted it to > > mean this is the case for which the "Use the latter option" directive was > > intended to apply. (-idirafter has been designated as preferable to > > -include-directory-after). > > Yeah, I get why you did it like this. It's just that the end effect > is to make --include-barrier seem less disparaged than the other > --include-* options, but I'm not sure there's supposed to be any > difference between them in that respect. > > Perhaps we should drop the "Use the latter option instead." thing > altogether for aliases. I'm not sure that it really helps, and this > thread shows that adding it automatically can lead to some odd corner > cases. > > In practice we shouldn't remove any of these aliases unless we're > also removing the option that they're an alias of. And if we do that, > the options should go through the usual deprecation cycle, just like > options without aliases. > > If there are specific options that we want to steer users away > from without deprecation, then we should probably have a specific > tag for that. Thanks, it makes sense to me. That would amount to changing just one line of the patch then, so it would look instead like the attached. -Lewis gcc/ChangeLog: 2020-03-19 Lewis Hyatt <lhyatt@gmail.com> * opts.c (print_filtered_help): Improve the help text for alias options.
Lewis Hyatt <lhyatt@gmail.com> writes: > On Tue, Mar 17, 2020 at 11:52:13AM +0000, Richard Sandiford wrote: >> Lewis Hyatt <lhyatt@gmail.com> writes: >> > On Mon, Mar 16, 2020 at 06:11:08PM +0000, Richard Sandiford wrote: >> >> Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >> > ... >> >> > FWIW there are three other options currently affected by this change >> >> > (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for >> >> > -Wimplicit-fallthrough I think is particularly helpful: >> >> > >> >> > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. >> >> > becomes >> >> > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). >> >> >> >> I also see: >> >> >> >> - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. >> >> + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). >> >> -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. >> >> ... >> >> --imacros Same as -imacros. Use the latter option instead. >> >> --imacros= Same as -imacros. Use the latter option instead. >> >> --include Same as -include. Use the latter option instead. >> >> - --include-barrier Same as -I. Use the latter option instead. >> >> + --include-barrier Same as -I-. >> >> --include-directory Same as -I. Use the latter option instead. >> >> --include-directory-after Same as -idirafter. Use the latter option instead. >> >> --include-directory-after= Same as -idirafter. Use the latter option instead. >> >> ... >> >> - -Wnormalized Same as -Wnormalized=. Use the latter option instead. >> >> + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). >> >> -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. >> >> >> >> I agree all of these look like improvements, especially the >> >> --include-barrier one. But I think the include ones also show >> >> that the "Use the latter option instead." decision is independent >> >> of whether the option is defined to be an alias. >> >> Gah, I meant an Alias() with an argument here. >> >> >> >> >> FWIW, there's also: >> >> >> >> Wmissing-format-attribute >> >> C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) >> >> ; >> >> >> >> which still ends up as: >> >> >> >> -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. >> >> >> >> Not really my area though, so I don't have any specific suggestion >> >> about how to separate the cases. >> >> >> > >> > Right, sorry, in my first email I only mentioned the options output by >> > --help=common, but there were a few more as well. Thanks very much for trying >> > it out and for the feedback. >> > >> > The rule I implemented was to change the help output for all alias options >> > with no documentation if they also specify the extra 2nd option (or 2nd and >> > 3rd options) to the Alias directive. For example, -include-barrier is like this: >> > >> > -include-barrier C ObjC C++ ObjC++ Alias(I, -) >> > >> > It serves to provide the argument '-' to the option '-I', so it is eligible for >> > the new text. The others are like this one: >> > >> > -include-directory-after C ObjC C++ ObjC++ Separate Alias(idirafter) MissingArgError(missing path after %qs) >> > >> > Since that one doesn't pass the extra args to Alias, I interpreted it to >> > mean this is the case for which the "Use the latter option" directive was >> > intended to apply. (-idirafter has been designated as preferable to >> > -include-directory-after). >> >> Yeah, I get why you did it like this. It's just that the end effect >> is to make --include-barrier seem less disparaged than the other >> --include-* options, but I'm not sure there's supposed to be any >> difference between them in that respect. >> >> Perhaps we should drop the "Use the latter option instead." thing >> altogether for aliases. I'm not sure that it really helps, and this >> thread shows that adding it automatically can lead to some odd corner >> cases. >> >> In practice we shouldn't remove any of these aliases unless we're >> also removing the option that they're an alias of. And if we do that, >> the options should go through the usual deprecation cycle, just like >> options without aliases. >> >> If there are specific options that we want to steer users away >> from without deprecation, then we should probably have a specific >> tag for that. > > Thanks, it makes sense to me. That would amount to changing just one line of > the patch then, so it would look instead like the attached. > > -Lewis > > gcc/ChangeLog: > > 2020-03-19 Lewis Hyatt <lhyatt@gmail.com> > > * opts.c (print_filtered_help): Improve the help text for alias > options. LGTM, thanks. OK if noone objects in 48 hrs. Richard > > commit 7a74dd55098e2ec8c2b87dc172ac34f91eefc0c1 > Author: Lewis Hyatt <lhyatt@gmail.com> > Date: Wed Feb 12 13:52:28 2020 -0500 > > driver: Improve the generated help text for alias options > > diff --git a/gcc/opts.c b/gcc/opts.c > index ac160ed8404..5dc7d65dedd 100644 > --- a/gcc/opts.c > +++ b/gcc/opts.c > @@ -1315,14 +1315,31 @@ print_filtered_help (unsigned int include_flags, > if (option->alias_target < N_OPTS > && cl_options [option->alias_target].help) > { > + const struct cl_option *target = cl_options + option->alias_target; > if (option->help == NULL) > { > - /* For undocumented options that are aliases for other options > - that are documented, point the reader to the other option in > - preference of the former. */ > - snprintf (new_help, sizeof new_help, > - _("Same as %s. Use the latter option instead."), > - cl_options [option->alias_target].opt_text); > + /* The option is undocumented but is an alias for an option that > + is documented. If the option has alias arguments, then its > + purpose is to provide certain arguments to the other option, so > + inform the reader of this. Otherwise, point the reader to the > + other option in preference to the former. */ > + > + if (option->alias_arg) > + { > + if (option->neg_alias_arg) > + snprintf (new_help, sizeof new_help, > + _("Same as %s%s (or, in negated form, %s%s)."), > + target->opt_text, option->alias_arg, > + target->opt_text, option->neg_alias_arg); > + else > + snprintf (new_help, sizeof new_help, > + _("Same as %s%s."), > + target->opt_text, option->alias_arg); > + } > + else > + snprintf (new_help, sizeof new_help, > + _("Same as %s."), > + target->opt_text); > } > else > {
On 3/17/20 5:52 AM, Richard Sandiford wrote: > Lewis Hyatt <lhyatt@gmail.com> writes: >> On Mon, Mar 16, 2020 at 06:11:08PM +0000, Richard Sandiford wrote: >>> Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >> ... >>>> FWIW there are three other options currently affected by this change >>>> (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for >>>> -Wimplicit-fallthrough I think is particularly helpful: >>>> >>>> -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. >>>> becomes >>>> -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). >>> >>> I also see: >>> >>> - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. >>> + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). >>> -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. >>> ... >>> --imacros Same as -imacros. Use the latter option instead. >>> --imacros= Same as -imacros. Use the latter option instead. >>> --include Same as -include. Use the latter option instead. >>> - --include-barrier Same as -I. Use the latter option instead. >>> + --include-barrier Same as -I-. >>> --include-directory Same as -I. Use the latter option instead. >>> --include-directory-after Same as -idirafter. Use the latter option instead. >>> --include-directory-after= Same as -idirafter. Use the latter option instead. >>> ... >>> - -Wnormalized Same as -Wnormalized=. Use the latter option instead. >>> + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). >>> -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. >>> >>> I agree all of these look like improvements, especially the >>> --include-barrier one. But I think the include ones also show >>> that the "Use the latter option instead." decision is independent >>> of whether the option is defined to be an alias. > > Gah, I meant an Alias() with an argument here. > >>> >>> FWIW, there's also: >>> >>> Wmissing-format-attribute >>> C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) >>> ; >>> >>> which still ends up as: >>> >>> -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. >>> >>> Not really my area though, so I don't have any specific suggestion >>> about how to separate the cases. >>> >> >> Right, sorry, in my first email I only mentioned the options output by >> --help=common, but there were a few more as well. Thanks very much for trying >> it out and for the feedback. >> >> The rule I implemented was to change the help output for all alias options >> with no documentation if they also specify the extra 2nd option (or 2nd and >> 3rd options) to the Alias directive. For example, -include-barrier is like this: >> >> -include-barrier C ObjC C++ ObjC++ Alias(I, -) >> >> It serves to provide the argument '-' to the option '-I', so it is eligible for >> the new text. The others are like this one: >> >> -include-directory-after C ObjC C++ ObjC++ Separate Alias(idirafter) MissingArgError(missing path after %qs) >> >> Since that one doesn't pass the extra args to Alias, I interpreted it to >> mean this is the case for which the "Use the latter option" directive was >> intended to apply. (-idirafter has been designated as preferable to >> -include-directory-after). > > Yeah, I get why you did it like this. It's just that the end effect > is to make --include-barrier seem less disparaged than the other > --include-* options, but I'm not sure there's supposed to be any > difference between them in that respect. > > Perhaps we should drop the "Use the latter option instead." thing > altogether for aliases. I'm not sure that it really helps, and this > thread shows that adding it automatically can lead to some odd corner > cases. > > In practice we shouldn't remove any of these aliases unless we're > also removing the option that they're an alias of. And if we do that, > the options should go through the usual deprecation cycle, just like > options without aliases. > > If there are specific options that we want to steer users away > from without deprecation, then we should probably have a specific > tag for that. The "Use the latter option" text was the outcome of the discussion of the patch for PR 68043: https://gcc.gnu.org/legacy-ml/gcc-patches/2015-10/msg01395.html where Joseph wanted to steer users toward the alternatives. I don't feel too strongly about it but reviewing the thread might be helpful. FWIW, an enhancement to consider is making use of colors in the output (under the same conditions as in diagnostics). That would make it possible to differentiate between recommended options and their discouraged or deprecated counterparts (among other things). Martin
Martin Sebor <msebor@gmail.com> writes: > On 3/17/20 5:52 AM, Richard Sandiford wrote: >> Lewis Hyatt <lhyatt@gmail.com> writes: >>> On Mon, Mar 16, 2020 at 06:11:08PM +0000, Richard Sandiford wrote: >>>> Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >>> ... >>>>> FWIW there are three other options currently affected by this change >>>>> (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for >>>>> -Wimplicit-fallthrough I think is particularly helpful: >>>>> >>>>> -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. >>>>> becomes >>>>> -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). >>>> >>>> I also see: >>>> >>>> - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. >>>> + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). >>>> -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. >>>> ... >>>> --imacros Same as -imacros. Use the latter option instead. >>>> --imacros= Same as -imacros. Use the latter option instead. >>>> --include Same as -include. Use the latter option instead. >>>> - --include-barrier Same as -I. Use the latter option instead. >>>> + --include-barrier Same as -I-. >>>> --include-directory Same as -I. Use the latter option instead. >>>> --include-directory-after Same as -idirafter. Use the latter option instead. >>>> --include-directory-after= Same as -idirafter. Use the latter option instead. >>>> ... >>>> - -Wnormalized Same as -Wnormalized=. Use the latter option instead. >>>> + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). >>>> -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. >>>> >>>> I agree all of these look like improvements, especially the >>>> --include-barrier one. But I think the include ones also show >>>> that the "Use the latter option instead." decision is independent >>>> of whether the option is defined to be an alias. >> >> Gah, I meant an Alias() with an argument here. >> >>>> >>>> FWIW, there's also: >>>> >>>> Wmissing-format-attribute >>>> C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) >>>> ; >>>> >>>> which still ends up as: >>>> >>>> -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. >>>> >>>> Not really my area though, so I don't have any specific suggestion >>>> about how to separate the cases. >>>> >>> >>> Right, sorry, in my first email I only mentioned the options output by >>> --help=common, but there were a few more as well. Thanks very much for trying >>> it out and for the feedback. >>> >>> The rule I implemented was to change the help output for all alias options >>> with no documentation if they also specify the extra 2nd option (or 2nd and >>> 3rd options) to the Alias directive. For example, -include-barrier is like this: >>> >>> -include-barrier C ObjC C++ ObjC++ Alias(I, -) >>> >>> It serves to provide the argument '-' to the option '-I', so it is eligible for >>> the new text. The others are like this one: >>> >>> -include-directory-after C ObjC C++ ObjC++ Separate Alias(idirafter) MissingArgError(missing path after %qs) >>> >>> Since that one doesn't pass the extra args to Alias, I interpreted it to >>> mean this is the case for which the "Use the latter option" directive was >>> intended to apply. (-idirafter has been designated as preferable to >>> -include-directory-after). >> >> Yeah, I get why you did it like this. It's just that the end effect >> is to make --include-barrier seem less disparaged than the other >> --include-* options, but I'm not sure there's supposed to be any >> difference between them in that respect. >> >> Perhaps we should drop the "Use the latter option instead." thing >> altogether for aliases. I'm not sure that it really helps, and this >> thread shows that adding it automatically can lead to some odd corner >> cases. >> >> In practice we shouldn't remove any of these aliases unless we're >> also removing the option that they're an alias of. And if we do that, >> the options should go through the usual deprecation cycle, just like >> options without aliases. >> >> If there are specific options that we want to steer users away >> from without deprecation, then we should probably have a specific >> tag for that. > > The "Use the latter option" text was the outcome of the discussion > of the patch for PR 68043: > https://gcc.gnu.org/legacy-ml/gcc-patches/2015-10/msg01395.html > where Joseph wanted to steer users toward the alternatives. I > don't feel too strongly about it but reviewing the thread might > be helpful. Thanks for the pointer. But I think Joseph's comment was more about not reproducing the documentation of the alias target: I also think it would be better just to give the "Same as" message without also repeating the description of the canonical option. on the basis that: Well, I think it might also encourage people to use the aliases, when for the most part we'd rather people used the canonical names (and so made it easier e.g. to search for other uses of the same option). And Lewis's patch is still doing that. It doesn't look like there was a specific request to add extra text to steer the user away from the alias. I can understand why adding that text seemed like a good idea, but I think Lewis's patch shows that it can also produce some oddities. IMO we should stick to what we know is correct: that the option is an alias of some other option. > FWIW, an enhancement to consider is making use of colors in > the output (under the same conditions as in diagnostics). That > would make it possible to differentiate between recommended > options and their discouraged or deprecated counterparts (among > other things). Yeah, sounds like it could be useful. Thanks, Richard
On 3/20/20 11:46 AM, Richard Sandiford wrote: > Martin Sebor <msebor@gmail.com> writes: >> On 3/17/20 5:52 AM, Richard Sandiford wrote: >>> Lewis Hyatt <lhyatt@gmail.com> writes: >>>> On Mon, Mar 16, 2020 at 06:11:08PM +0000, Richard Sandiford wrote: >>>>> Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: >>>> ... >>>>>> FWIW there are three other options currently affected by this change >>>>>> (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for >>>>>> -Wimplicit-fallthrough I think is particularly helpful: >>>>>> >>>>>> -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. >>>>>> becomes >>>>>> -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). >>>>> >>>>> I also see: >>>>> >>>>> - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. >>>>> + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). >>>>> -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. >>>>> ... >>>>> --imacros Same as -imacros. Use the latter option instead. >>>>> --imacros= Same as -imacros. Use the latter option instead. >>>>> --include Same as -include. Use the latter option instead. >>>>> - --include-barrier Same as -I. Use the latter option instead. >>>>> + --include-barrier Same as -I-. >>>>> --include-directory Same as -I. Use the latter option instead. >>>>> --include-directory-after Same as -idirafter. Use the latter option instead. >>>>> --include-directory-after= Same as -idirafter. Use the latter option instead. >>>>> ... >>>>> - -Wnormalized Same as -Wnormalized=. Use the latter option instead. >>>>> + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). >>>>> -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. >>>>> >>>>> I agree all of these look like improvements, especially the >>>>> --include-barrier one. But I think the include ones also show >>>>> that the "Use the latter option instead." decision is independent >>>>> of whether the option is defined to be an alias. >>> >>> Gah, I meant an Alias() with an argument here. >>> >>>>> >>>>> FWIW, there's also: >>>>> >>>>> Wmissing-format-attribute >>>>> C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) >>>>> ; >>>>> >>>>> which still ends up as: >>>>> >>>>> -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. >>>>> >>>>> Not really my area though, so I don't have any specific suggestion >>>>> about how to separate the cases. >>>>> >>>> >>>> Right, sorry, in my first email I only mentioned the options output by >>>> --help=common, but there were a few more as well. Thanks very much for trying >>>> it out and for the feedback. >>>> >>>> The rule I implemented was to change the help output for all alias options >>>> with no documentation if they also specify the extra 2nd option (or 2nd and >>>> 3rd options) to the Alias directive. For example, -include-barrier is like this: >>>> >>>> -include-barrier C ObjC C++ ObjC++ Alias(I, -) >>>> >>>> It serves to provide the argument '-' to the option '-I', so it is eligible for >>>> the new text. The others are like this one: >>>> >>>> -include-directory-after C ObjC C++ ObjC++ Separate Alias(idirafter) MissingArgError(missing path after %qs) >>>> >>>> Since that one doesn't pass the extra args to Alias, I interpreted it to >>>> mean this is the case for which the "Use the latter option" directive was >>>> intended to apply. (-idirafter has been designated as preferable to >>>> -include-directory-after). >>> >>> Yeah, I get why you did it like this. It's just that the end effect >>> is to make --include-barrier seem less disparaged than the other >>> --include-* options, but I'm not sure there's supposed to be any >>> difference between them in that respect. >>> >>> Perhaps we should drop the "Use the latter option instead." thing >>> altogether for aliases. I'm not sure that it really helps, and this >>> thread shows that adding it automatically can lead to some odd corner >>> cases. >>> >>> In practice we shouldn't remove any of these aliases unless we're >>> also removing the option that they're an alias of. And if we do that, >>> the options should go through the usual deprecation cycle, just like >>> options without aliases. >>> >>> If there are specific options that we want to steer users away >>> from without deprecation, then we should probably have a specific >>> tag for that. >> >> The "Use the latter option" text was the outcome of the discussion >> of the patch for PR 68043: >> https://gcc.gnu.org/legacy-ml/gcc-patches/2015-10/msg01395.html >> where Joseph wanted to steer users toward the alternatives. I >> don't feel too strongly about it but reviewing the thread might >> be helpful. > > Thanks for the pointer. But I think Joseph's comment was more about > not reproducing the documentation of the alias target: > > I also think it would be better just to give the "Same as" message > without also repeating the description of the canonical option. > > on the basis that: > > Well, I think it might also encourage people to use the aliases, when > for the most part we'd rather people used the canonical names (and so > made it easier e.g. to search for other uses of the same option). > > And Lewis's patch is still doing that. It doesn't look like there was > a specific request to add extra text to steer the user away from the alias. > > I can understand why adding that text seemed like a good idea, > but I think Lewis's patch shows that it can also produce some oddities. > IMO we should stick to what we know is correct: that the option is an > alias of some other option. Sure. I have no objection. I just wanted to give some background on why the code is the way it is. FWIW, I expect the majority or users, especially those looking for help with options, will refer to the more verbose text in the manual far more often than to the --help output. I think any guidance we give will be more effective in the former. Martin > >> FWIW, an enhancement to consider is making use of colors in >> the output (under the same conditions as in diagnostics). That >> would make it possible to differentiate between recommended >> options and their discouraged or deprecated counterparts (among >> other things). > > Yeah, sounds like it could be useful. > > Thanks, > Richard >
On Fri, Mar 20, 2020 at 01:16:42PM -0600, Martin Sebor wrote: > On 3/20/20 11:46 AM, Richard Sandiford wrote: > > Martin Sebor <msebor@gmail.com> writes: > > > On 3/17/20 5:52 AM, Richard Sandiford wrote: > > > > Lewis Hyatt <lhyatt@gmail.com> writes: > > > > > On Mon, Mar 16, 2020 at 06:11:08PM +0000, Richard Sandiford wrote: > > > > > > Lewis Hyatt via Gcc-patches <gcc-patches@gcc.gnu.org> writes: > > > > > ... > > > > > > > FWIW there are three other options currently affected by this change > > > > > > > (-Wimplicit-fallthrough, -fcf-protection, and -flive-patching). The change for > > > > > > > -Wimplicit-fallthrough I think is particularly helpful: > > > > > > > > > > > > > > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=. Use the latter option instead. > > > > > > > becomes > > > > > > > -Wimplicit-fallthrough Same as -Wimplicit-fallthrough=3 (or, in negated form, -Wimplicit-fallthrough=0). > > > > > > > > > > > > I also see: > > > > > > > > > > > > - -ftail-call-workaround Same as -ftail-call-workaround=. Use the latter option instead. > > > > > > + -ftail-call-workaround Same as -ftail-call-workaround=1 (or, in negated form, -ftail-call-workaround=0). > > > > > > -ftail-call-workaround=<0,2> Disallow tail call optimization when a calling routine may have omitted character lengths. > > > > > > ... > > > > > > --imacros Same as -imacros. Use the latter option instead. > > > > > > --imacros= Same as -imacros. Use the latter option instead. > > > > > > --include Same as -include. Use the latter option instead. > > > > > > - --include-barrier Same as -I. Use the latter option instead. > > > > > > + --include-barrier Same as -I-. > > > > > > --include-directory Same as -I. Use the latter option instead. > > > > > > --include-directory-after Same as -idirafter. Use the latter option instead. > > > > > > --include-directory-after= Same as -idirafter. Use the latter option instead. > > > > > > ... > > > > > > - -Wnormalized Same as -Wnormalized=. Use the latter option instead. > > > > > > + -Wnormalized Same as -Wnormalized=nfc (or, in negated form, -Wnormalized=none). > > > > > > -Wnormalized=[none|id|nfc|nfkc] Warn about non-normalized Unicode strings. > > > > > > > > > > > > I agree all of these look like improvements, especially the > > > > > > --include-barrier one. But I think the include ones also show > > > > > > that the "Use the latter option instead." decision is independent > > > > > > of whether the option is defined to be an alias. > > > > > > > > Gah, I meant an Alias() with an argument here. > > > > > > > > > > > > > > > > FWIW, there's also: > > > > > > > > > > > > Wmissing-format-attribute > > > > > > C ObjC C++ ObjC++ Warning Alias(Wsuggest-attribute=format) > > > > > > ; > > > > > > > > > > > > which still ends up as: > > > > > > > > > > > > -Wmissing-format-attribute Same as -Wsuggest-attribute=format. Use the latter option instead. > > > > > > > > > > > > Not really my area though, so I don't have any specific suggestion > > > > > > about how to separate the cases. > > > > > > > > > > > > > > > > Right, sorry, in my first email I only mentioned the options output by > > > > > --help=common, but there were a few more as well. Thanks very much for trying > > > > > it out and for the feedback. > > > > > > > > > > The rule I implemented was to change the help output for all alias options > > > > > with no documentation if they also specify the extra 2nd option (or 2nd and > > > > > 3rd options) to the Alias directive. For example, -include-barrier is like this: > > > > > > > > > > -include-barrier C ObjC C++ ObjC++ Alias(I, -) > > > > > > > > > > It serves to provide the argument '-' to the option '-I', so it is eligible for > > > > > the new text. The others are like this one: > > > > > > > > > > -include-directory-after C ObjC C++ ObjC++ Separate Alias(idirafter) MissingArgError(missing path after %qs) > > > > > > > > > > Since that one doesn't pass the extra args to Alias, I interpreted it to > > > > > mean this is the case for which the "Use the latter option" directive was > > > > > intended to apply. (-idirafter has been designated as preferable to > > > > > -include-directory-after). > > > > > > > > Yeah, I get why you did it like this. It's just that the end effect > > > > is to make --include-barrier seem less disparaged than the other > > > > --include-* options, but I'm not sure there's supposed to be any > > > > difference between them in that respect. > > > > > > > > Perhaps we should drop the "Use the latter option instead." thing > > > > altogether for aliases. I'm not sure that it really helps, and this > > > > thread shows that adding it automatically can lead to some odd corner > > > > cases. > > > > > > > > In practice we shouldn't remove any of these aliases unless we're > > > > also removing the option that they're an alias of. And if we do that, > > > > the options should go through the usual deprecation cycle, just like > > > > options without aliases. > > > > > > > > If there are specific options that we want to steer users away > > > > from without deprecation, then we should probably have a specific > > > > tag for that. > > > > > > The "Use the latter option" text was the outcome of the discussion > > > of the patch for PR 68043: > > > https://gcc.gnu.org/legacy-ml/gcc-patches/2015-10/msg01395.html > > > where Joseph wanted to steer users toward the alternatives. I > > > don't feel too strongly about it but reviewing the thread might > > > be helpful. > > > > Thanks for the pointer. But I think Joseph's comment was more about > > not reproducing the documentation of the alias target: > > > > I also think it would be better just to give the "Same as" message > > without also repeating the description of the canonical option. > > > > on the basis that: > > > > Well, I think it might also encourage people to use the aliases, when > > for the most part we'd rather people used the canonical names (and so > > made it easier e.g. to search for other uses of the same option). > > > > And Lewis's patch is still doing that. It doesn't look like there was > > a specific request to add extra text to steer the user away from the alias. > > > > I can understand why adding that text seemed like a good idea, > > but I think Lewis's patch shows that it can also produce some oddities. > > IMO we should stick to what we know is correct: that the option is an > > alias of some other option. > > Sure. I have no objection. I just wanted to give some background > on why the code is the way it is. > > FWIW, I expect the majority or users, especially those looking for > help with options, will refer to the more verbose text in the manual > far more often than to the --help output. I think any guidance we > give will be more effective in the former. Yeah, agreed that most users would be looking at the manual. Hopefully this is still a small improvement. Thank you both for considering it! I will plan to push it in a couple days unless I hear otherwise. -Lewis
diff --git a/gcc/opts.c b/gcc/opts.c index ac160ed8404..a120858d77b 100644 --- a/gcc/opts.c +++ b/gcc/opts.c @@ -1315,14 +1315,31 @@ print_filtered_help (unsigned int include_flags, if (option->alias_target < N_OPTS && cl_options [option->alias_target].help) { + const struct cl_option *target = cl_options + option->alias_target; if (option->help == NULL) { - /* For undocumented options that are aliases for other options - that are documented, point the reader to the other option in - preference of the former. */ - snprintf (new_help, sizeof new_help, - _("Same as %s. Use the latter option instead."), - cl_options [option->alias_target].opt_text); + /* The option is undocumented but is an alias for an option that + is documented. If the option has alias arguments, then its + purpose is to provide certain arguments to the other option, so + inform the reader of this. Otherwise, point the reader to the + other option in preference to the former. */ + + if (option->alias_arg) + { + if (option->neg_alias_arg) + snprintf (new_help, sizeof new_help, + _("Same as %s%s (or, in negated form, %s%s)."), + target->opt_text, option->alias_arg, + target->opt_text, option->neg_alias_arg); + else + snprintf (new_help, sizeof new_help, + _("Same as %s%s."), + target->opt_text, option->alias_arg); + } + else + snprintf (new_help, sizeof new_help, + _("Same as %s. Use the latter option instead."), + target->opt_text); } else {