Re: [PATCH] driver: Improve the generated help text for alias options with arguments

[Lewis Hyatt <lhyatt@gmail.com>]

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