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

[Volker Reichelt <v.reichelt@netcologne.de>]

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.

> Hope this is constructive.

Yes, it is!

> Dave