[PATCH] New C++ warning option '-Wduplicated-access-specifiers'

[David Malcolm]

On Fri, 2017-07-21 at 19:58 +0200, Volker Reichelt wrote:
> On 21 Jul, David Malcolm wrote:
> > On Thu, 2017-07-20 at 18:35 +0200, Volker Reichelt wrote:
> >> Hi,
> >> 
> >> the following patch introduces a new C++ warning option
> >> -Wduplicated-access-specifiers that warns about redundant
> >> access-specifiers in classes, e.g.
> >> 
> >>   class B
> >>   {
> >>     public:
> >>       B();
> >> 
> >>     private:
> >>       void foo();
> >>     private:
> >>       int i;
> >>   };
> >> 
> >> test.cc:8:5: warning: duplicate 'private' access-specifier [
> >> -Wduplicated-access-specifiers]
> >>      private:
> >>      ^~~~~~~
> >>      -------
> >> test.cc:6:5: note: access-specifier was previously given here
> >>      private:
> >>      ^~~~~~~
> > 
> > Thanks for working on this.
> > 
> > I'm not able to formally review, but you did CC me; various
> comments below throughout.
> > 
> >> The test is implemented by tracking the location of the last
> >> access-specifier together with the access-specifier itself.
> >> The location serves two purposes: the obvious one is to be able to
> >> print the location in the note that comes with the warning.
> >> The second purpose is to be able to distinguish an access
> -specifier
> >> given by the user from the default of the class type (i.e. public
> for
> >> 'struct' and private for 'class') where the location is set to
> >> UNKNOWN_LOCATION. The warning is only issued if the user gives the
> >> access-specifier twice, i.e. if the previous location is not
> >> UNKNOWN_LOCATION.
> > 
> > Presumably given
> > 
> > struct foo
> > {
> > public:
> >   /* ... *
> > };
> > 
> > we could issue something like:
> > 
> >   warning: access-specifier 'public' is redundant within 'struct'
> > 
> > for the first; similarly for:
> > 
> > class bar
> > {
> > private:
> > };
> > 
> > we could issue:
> > 
> >   warning: access-specifier 'private' is redundant within 'class'
> > 
> > 
> >> One could actually make this a two-level warning so that on the
> >> higher level also the default class-type settings are taken into
> >> account. Would that be helpful? I could prepare a second patch for
> >> that.
> > 
> > I'm not sure how best to structure it.
> > 
> > FWIW, when I first saw the patch, I wasn't a big fan of the
> warning, as I like to use access-specifiers to break up the kinds of
> entities within a class.
> > 
> > For example, our coding guidelines 
> >   https://gcc.gnu.org/codingconventions.html#Class_Form
> > recommend:
> > 
> > "first define all public types,
> > then define all non-public types,
> > then declare all public constructors,
> > ...
> > then declare all non-public member functions, and
> > then declare all non-public member variables."
> > 
> > I find it's useful to put a redundant "private:" between the last
> two,
> > e.g.:
> > 
> > class baz
> > {
> >  public:
> >   ...
> > 
> >  private:
> >   void some_private_member ();
> > 
> >  private:
> >   int m_some_field;
> > };
> > 
> > to provide a subdivision between the private member functions and
> the
> > private data fields.
> 
> That's what also can be seen in our libstdc++ to some extent.
> The other half of the warnings indicate redundant access-specifiers.
> 
> It's up to the user to keep those duplicate access-specifiers as
> subdividers or to use something else (like comments) to do that
> and to switch on the warning for her/his code.
> Because the subdivider usage seems to be relatively common,
> I don't want to enable the warning by -Wall or -Wextra.
> 
> > This might be a violation of our guidelines (though if so, I'm not
> sure
> > it's explicitly stated), but I find it useful, and the patch would
> warn
> > about it.
> > 
> > Having said that, looking at e.g. the "jit" subdir, I see lots of
> > places where the warning would fire.  In some of these, the code
> has a
> > bit of a "smell", so maybe I like the warning after all... in that
> it
> > can be good for a new warning to draw attention to code that might
> need
> > work.
> > 
> > Sorry that this is rambling; comments on the patch inline below.
> > 
> > Bootstrapped and regtested on x86_64-pc-linux-gnu.
> >> OK for trunk?
> >> 
> >> Btw, there are about 50 places in our libstdc++ headers where this
> >> warning triggers. I'll come up with a patch for this later.
> >> 
> >> Regards,
> >> Volker
> >> 
> >> 
> >> 2017-07-20  Volker Reichelt  <v.reichelt@netcologne.de>
> >> 
> >>         * doc/invoke.texi (-Wduplicated-access-specifiers):
> Document
> >> new
> >>         warning option.
> >> 
> >> Index: gcc/doc/invoke.texi
> >>
> ===================================================================
> >> --- gcc/doc/invoke.texi (revision 250356)
> >> +++ gcc/doc/invoke.texi (working copy)
> >> @@ -275,7 +275,7 @@
> >>  -Wdisabled-optimization @gol
> >>  -Wno-discarded-qualifiers  -Wno-discarded-array-qualifiers @gol
> >>  -Wno-div-by-zero  -Wdouble-promotion @gol
> >> --Wduplicated-branches  -Wduplicated-cond @gol
> >> +-Wduplicated-access-specifiers  -Wduplicated-branches  
> -Wduplicated
> >> -cond @gol
> >>  -Wempty-body  -Wenum-compare  -Wno-endif-labels  -Wexpansion-to
> >> -defined @gol
> >>  -Werror  -Werror=*  -Wextra-semi  -Wfatal-errors @gol
> >>  -Wfloat-equal  -Wformat  -Wformat=2 @gol
> >> @@ -5388,6 +5388,12 @@
> >>  
> >>  This warning is enabled by @option{-Wall}.
> >>  
> >> +@item -Wduplicated-access-specifiers
> >> +@opindex Wno-duplicated-access-specifiers
> >> +@opindex Wduplicated-access-specifiers
> >> +Warn when an access-specifier is redundant because it was already
> >> given
> >> +before.
> > 
> > Presumably this should be marked as C++-specific.
> 
> Oops, good point!
> 
> > I think it's best to give an example for any warning, though we
> don't
> > do that currently.
> 
> Sounds reasonable. Especially because 'access-specifier' is a very
> technical term that is not linked to 'public/protected/private'
> by everybody.
> 
> [...snip...]
> 
> >> Index: gcc/cp/parser.c
> >>
> ===================================================================
> >> --- gcc/cp/parser.c     (revision 250356)
> >> +++ gcc/cp/parser.c     (working copy)
> >> @@ -23113,8 +23113,24 @@
> >>         case RID_PRIVATE:
> >>           /* Consume the access-specifier.  */
> >>           cp_lexer_consume_token (parser->lexer);
> >> +
> >> +         /* Warn if the same access-specifier has been given
> >> already.  */
> >> +         if (warn_duplicated_access
> >> +             && current_access_specifier == token->u.value
> >> +             && current_access_specifier_loc != UNKNOWN_LOCATION)
> >> +           {
> >> +             gcc_rich_location richloc (token->location);
> >> +             richloc.add_fixit_remove ();
> > 
> > If the fix-it hint is to work, it presumably needs to remove two
> > tokens: both the "private" (or whatever), *and* the colon.
> 
> I did not do this because I did not want to reorder the code.
> OTOH just moving the cp_parser_require line a little bit up
> should not be a big deal for better diagnostics.
> 
> > You can probably do this via:
> > 
> >   richloc.add_fixit_remove ();  /* for the primary location */
> >   richloc.add_fixit_remove (colon_loc);  /* for the colon */
> > 
> > and the rich_location ought to do the right thing, consolidating
> the removals if they are adjacent (and coping with e.g. a comment
> between them if they're not).
> >
> >> +             warning_at_rich_loc (&richloc,
> >> OPT_Wduplicated_access_specifiers,
> >> +                                  "duplicate %qE access
> -specifier",
> >> +                                  token->u.value);
> >> +             inform (current_access_specifier_loc,
> >> +                     "access-specifier was previously given
> here");
> >> +           }
> >> +
> >>           /* Remember which access-specifier is active.  */
> >>           current_access_specifier = token->u.value;
> >> +         current_access_specifier_loc = token->location;
> >>           /* Look for the `:'.  */
> >>           cp_parser_require (parser, CPP_COLON, RT_COLON);
> >>           break;
> >>
> ===================================================================
> >> 
> >> 
> >> 2017-07-20  Volker Reichelt  <v.reichelt@netcologne.de>
> >> 
> >>         * g++.dg/warn/Wduplicated-access-specifiers-1.C: New test.
> 
> {...snip...]
> 
> > If you're going to implement a fix-it hint for this, there should
> be a
> > test case that tests it (probably as a separate file, e.g.
> Wduplicated
> > -access-specifiers-2.C, to allow for the extra option -
> -fdiagnostics
> > -show-caret, covering just one instance of the diagnostic firing,
> for
> > simplicity).
> 
> I actually did try that, but dejagnu somehow gets confused.
> To me it looks like the reason for this is that both multi-line
> messages
> are so similar. I'll give it a second try, though.

I'm not sure what you mean by "both" multi-line messages; shouldn't
there be just one multi-line message for the fix-it hint.

Isn't this like e.g. g++.dg/cpp0x/auto1.C ?  (an example of a testcase
that verifies a deletion fix-it hint)

Dave