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

[David Malcolm <dmalcolm at redhat dot com>]

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.

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.

I think it's best to give an example for any warning, though we don't
do that currently.

>  @item -Wduplicated-branches
>  @opindex Wno-duplicated-branches
>  @opindex Wduplicated-branches
> ===================================================================
> 
> 
> 2017-07-20  Volker Reichelt  <v.reichelt@netcologne.de>
> 
>         * c.opt (Wduplicated-access-specifiers): New C++ warning
> flag.
> 
> Index: gcc/c-family/c.opt
> ===================================================================
> --- gcc/c-family/c.opt  (revision 250356)
> +++ gcc/c-family/c.opt  (working copy)
> @@ -476,6 +476,10 @@
>  C ObjC C++ ObjC++ Var(warn_div_by_zero) Init(1) Warning
>  Warn about compile-time integer division by zero.
>  
> +Wduplicated-access-specifiers
> +C++ ObjC++ Var(warn_duplicated_access) Warning
> +Warn about duplicated access-specifiers.
> +
>  Wduplicated-branches
>  C ObjC C++ ObjC++ Var(warn_duplicated_branches) Init(0) Warning
>  Warn about duplicated branches in if-else statements.
> ===================================================================
> 
> 
> 2017-07-20  Volker Reichelt  <v.reichelt@netcologne.de>
> 
>         * cp-tree.h (struct saved_scope): New access_specifier_loc
> variable.
>         (current_access_specifier_loc): New macro.
>         * class.c (struct class_stack_node): New access_loc variable.
>         (pushclass): Push current_access_specifier_loc.  Set new
>         initial value.
>         (popclass): Pop current_access_specifier_loc.
>         * parser.c (cp_parser_member_specification_opt): Warn about
>         duplicated access-specifier.  Set
> current_access_specifier_loc.
> 
> Index: gcc/cp/cp-tree.h
> ===================================================================
> --- gcc/cp/cp-tree.h    (revision 250356)
> +++ gcc/cp/cp-tree.h    (working copy)
> @@ -1531,6 +1531,7 @@
>    tree class_name;
>    tree class_type;
>    tree access_specifier;
> +  source_location access_specifier_loc;
>    tree function_decl;
>    vec<tree, va_gc> *lang_base;
>    tree lang_name;
> @@ -1592,6 +1593,7 @@
>     class, or union).  */
>  
>  #define current_access_specifier scope_chain->access_specifier
> +#define current_access_specifier_loc scope_chain
> ->access_specifier_loc
>  
>  /* Pointer to the top of the language name stack.  */
>  
> Index: gcc/cp/class.c
> ===================================================================
> --- gcc/cp/class.c      (revision 250356)
> +++ gcc/cp/class.c      (working copy)
> @@ -60,6 +60,7 @@
>    /* The access specifier pending for new declarations in the scope
> of
>       this class.  */
>    tree access;
> +  location_t access_loc;
>  
>    /* If were defining TYPE, the names used in this class.  */
>    splay_tree names_used;
> @@ -7723,6 +7724,7 @@
>    csn->name = current_class_name;
>    csn->type = current_class_type;
>    csn->access = current_access_specifier;
> +  csn->access_loc = current_access_specifier_loc;
>    csn->names_used = 0;
>    csn->hidden = 0;
>    current_class_depth++;
> @@ -7738,6 +7740,7 @@
>    current_access_specifier = (CLASSTYPE_DECLARED_CLASS (type)
>                               ? access_private_node
>                               : access_public_node);
> +  current_access_specifier_loc = UNKNOWN_LOCATION;
>  
>    if (previous_class_level
>        && type != previous_class_level->this_entity
> @@ -7777,6 +7780,7 @@
>    current_class_name =
> current_class_stack[current_class_depth].name;
>    current_class_type =
> current_class_stack[current_class_depth].type;
>    current_access_specifier =
> current_class_stack[current_class_depth].access;
> +  current_access_specifier_loc =
> current_class_stack[current_class_depth].access_loc;
>    if (current_class_stack[current_class_depth].names_used)
>      splay_tree_delete
> (current_class_stack[current_class_depth].names_used);
>  }
> 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.

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.
> 
> Index: gcc/testsuite/g++.dg/warn/Wduplicated-access-specifiers-1.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/Wduplicated-access-specifiers-1.C 2017
> -07-20
> +++ gcc/testsuite/g++.dg/warn/Wduplicated-access-specifiers-1.C 2017
> -07-20
> @@ -0,0 +1,35 @@
> +// { dg-options "-Wduplicated-access-specifiers" }
> +
> +struct A
> +{
> +    int i;
> +  public:       // { dg-message "previously" }
> +    int j;
> +  public:       // { dg-warning "access-specifier" }
> +    int k;
> +  protected:    // { dg-message "previously" }
> +
> +    class B
> +    {
> +      private:  // { dg-message "previously" }
> +      private:  // { dg-warning "access-specifier" }
> +      public:
> +    };
> +
> +  protected:    // { dg-warning "access-specifier" }
> +    void a();
> +  public:
> +    void b();
> +  private:      // { dg-message "previously" }
> +    void c();
> +  private:      // { dg-warning "access-specifier" }
> +    void d();
> +  public:
> +    void e();
> +  protected:    // { dg-message "previously" }
> +    void f();
> +  protected:    // { dg-warning "access-specifier" }
> +                // { dg-message "previously" "" { target *-*-* } .-1
> }
> +    void g();
> +  protected:    // { dg-warning "access-specifier" }
> +};

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).

Hope this is constructive.
Dave