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

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

[Volker Reichelt]

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:
     ^~~~~~~

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.

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.

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.
+
 @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 ();
+	      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" }
+};

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

[David Malcolm]

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

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

[David Malcolm]

On Fri, 2017-07-21 at 11:56 -0400, 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.

Maybe combine the two ideas, and call it
  -Wredundant-access-specifiers
?

Or just "-Waccess-specifiers" ?

[...snip...]

Dave

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

[Volker Reichelt]

On 21 Jul, David Malcolm wrote:
> On Fri, 2017-07-21 at 11:56 -0400, 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.
> 
> Maybe combine the two ideas, and call it
>   -Wredundant-access-specifiers
> ?
> 
> Or just "-Waccess-specifiers" ?
> 
> [...snip...]
> 
> Dave

Thanks for having a look at this!

My favorite version would be to use '-Waccess-specifiers=1' for the
original warning and '-Waccess-specifiers=2' for the stricter version
that also checks against the class-type default.

We could then let '-Waccess-specifiers' default to any of those two.
I'm afraid that level 2 will fire way too often for legacy code
(and there might be coding conventions that require an access specifier
at the beginning of a class/struct). So I'd vote for level 1 as default.

The last argument is also the reason why I'd like to see a two-lveel
warning instead of just different error messages (although these are
very helpful for seeing what's really goiing on with your code).

What do you think?

Regards,
Volker

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

[Martin Sebor]

On 07/20/2017 10:35 AM, 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;
>   };

I'm very fond of warnings that help find real bugs, or even that
provide an opportunity to review code for potential problems or
inefficiencies and suggest a possibility to improve it in some
way (make it clearer, or easier for humans or compilers to find
real bugs in, or faster, etc.), even if the code isn't strictly
incorrect.

In this case I'm having trouble coming up with an example where
the warning would have this effect.  What do you have in mind?
(Duplicate access specifiers tend to crop up in large classes
and/or as a result of preprocessor conditionals.)

Btw., there are examples of poor coding practices where I can
imagine a warning focused on access specifiers being helpful.
For instance, a class whose member functions maintain non-trivial
invariants among its data members should declare the data private
to prevent clients from inadvertently breaking those invariants.
A specific example might be a container class (like string or
vector) that owns the block of memory it allocates.  A warning
that detected when such members are publicly accessible could
help improve encapsulation.  I suspect this would be challenging
to implement and would likely require some non-trivial analysis
in the middle end.

Another example is a class that defines an inaccessible member
that isn't used (e.g., class A { int i; }; that Clang detects
with -Wunused-private-field; or class A { void f () { } };).
I would expect this to be doable in the front end.

Martin

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

[Volker Reichelt]

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

Re: [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

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

[Volker Reichelt]

On 21 Jul, Martin Sebor wrote:
> On 07/20/2017 10:35 AM, 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;
>>   };
> 
> I'm very fond of warnings that help find real bugs, or even that
> provide an opportunity to review code for potential problems or
> inefficiencies and suggest a possibility to improve it in some
> way (make it clearer, or easier for humans or compilers to find
> real bugs in, or faster, etc.), even if the code isn't strictly
> incorrect.
> 
> In this case I'm having trouble coming up with an example where
> the warning would have this effect.  What do you have in mind?
> (Duplicate access specifiers tend to crop up in large classes
> and/or as a result of preprocessor conditionals.)

This warning fights the "tend to crop up" effect that clutters the
code. After some time these redundant access specifiers creep in
and make your code harder to read. If I see something like

  ...
    void foo() const;

  private:
    void bar();
  ...

on the screen I tend to think that 'foo' has a different access
level than bar. If that's not the case because the access-specifier
is redundant, then that's just confusing and distracting.
The warning helps to maintain readability of your code.

The benefit might not be big, but the warning comes at relatively
low cost. It passes a location around through the class stack and
checks less than a handful of tokens.

My personal motivation to implement this warning was the fact that
I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT
mechanism to the new function pointer syntax of Qt5. In the old
version you had to mark slots in the following fashion:

  public slots:
    void foo();
    void bar();

But now you can use any function making the 'slots' macro obsolete.
Therefore I ended up with hundreds of redundant access-specifiers
which this warning helped to clean up. Doing this sort of thing in the
compiler with a proper parser is way safer than to write some script
to achieve this.

Btw, the SonarAnalyzer also provides this warning to clean up code:

https://sonarcloud.io/organizations/default/rules#rule_key=cpp%3AS3539

> Btw., there are examples of poor coding practices where I can
> imagine a warning focused on access specifiers being helpful.
> For instance, a class whose member functions maintain non-trivial
> invariants among its data members should declare the data private
> to prevent clients from inadvertently breaking those invariants.
> A specific example might be a container class (like string or
> vector) that owns the block of memory it allocates.  A warning
> that detected when such members are publicly accessible could
> help improve encapsulation.  I suspect this would be challenging
> to implement and would likely require some non-trivial analysis
> in the middle end.

That's way beyond the scope of what my warning is trying to achieve.

> Another example is a class that defines an inaccessible member
> that isn't used (e.g., class A { int i; }; that Clang detects
> with -Wunused-private-field; or class A { void f () { } };).
> I would expect this to be doable in the front end.

That's indeed a nice warning, too.

> Martin

Regards,
Volker

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

[Volker Reichelt]

On 21 Jul, David Malcolm wrote:
> 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:
>> >>      ^~~~~~~

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

There are two multi-line messages. One for the warning and one for the
note, see the example above the "[...snip...]". The message after the
note consists of the first two lines of the message after the warning.
This seems to confuse dejagnu. However, I managed to work around this
problem. I'll post an updated patch soon.

Regards,
Volker

[PATCH v2] New C++ warning option '-Waccess-specifiers'

[Volker Reichelt]

Hi again,

here is an updated patch for a new warning about redundant
access-specifiers. It takes Dave's various comments into account.

The main changes w.r.t. to the previous versions are:

* The warning is now a two-level warning with a slightly shorter name:
  -Waccess-specifiers=1, -Waccess-specifiers=2
  with -Waccess-specifiers defaulting to -Waccess-specifiers=1.
* The warning now checks for 3 different scenarios, see testcase
  Waccess-specifiers-2.C below:
  A) Two consecutive access-specifiers, so that the first one
     has no effect. (This is new.)
  B) Two (non-consecutive) equal access-specifiers.
     (That's what the original patch checked.)
  C) An access-specifier that does not change the class-type's default.
     (That's what I only suggested in the original patch.)
  The first two tests are enabled in level 1, the last in level 2.
* The warning is now in a separate function.
* The fix-it hints now include the colon after the access-specifier.
* There's a testcase for the fix-it hints.

What's missing from Dave's comments are some examples for invoke.texi.
I'll work on that once the warning made it into the trunk.

The switch statement in cp_parser_maybe_warn_access_specifier is a little
bit ugly. It would be nicer to emit the warnings directly next to the
checks. But then I'd have to write the code for the fix-it hint 3 times.
With C++11 I'd use a lambda for this, but with C++03 I hope the switch
is OK.

Bootstrapped and regtested on x86_64-pc-linux-gnu.
OK for trunk?

Regards,
Volker


2017-07-22  Volker Reichelt  <v.reichelt@netcologne.de>

	* doc/invoke.texi (-Waccess-specifiers, -Waccess-specifiers=):
	Document new warning options.

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 250356)
+++ gcc/doc/invoke.texi	(working copy)
@@ -259,7 +259,8 @@
 @xref{Warning Options,,Options to Request or Suppress Warnings}.
 @gccoptlist{-fsyntax-only  -fmax-errors=@var{n}  -Wpedantic @gol
 -pedantic-errors @gol
--w  -Wextra  -Wall  -Waddress  -Waggregate-return  @gol
+-w  -Wextra  -Wall  -Waccess-specifiers  -Waccess-specifiers=@var{n} @gol
+-Waddress  -Waggregate-return  @gol
 -Walloc-zero  -Walloc-size-larger-than=@var{n}
 -Walloca  -Walloca-larger-than=@var{n} @gol
 -Wno-aggressive-loop-optimizations  -Warray-bounds  -Warray-bounds=@var{n} @gol
@@ -5254,6 +5255,13 @@
 Warn about overriding virtual functions that are not marked with the override
 keyword.
 
+@item -Waccess-specifiers @r{(C++ and Objective-C++ only)}
+@itemx -Waccess-specifiers=@var{n}
+@opindex Wno-access-specifiers
+@opindex Waccess-specifiers
+Warn when an access-specifier is redundant because it was already given
+before.
+
 @item -Walloc-zero
 @opindex Wno-alloc-zero
 @opindex Walloc-zero
===================================================================


2017-07-22  Volker Reichelt  <v.reichelt@netcologne.de>

	* c.opt (Waccess-specifiers=): New C++ warning flag.
	(Waccess-specifiers): New alias.

Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 250443)
+++ gcc/c-family/c.opt	(working copy)
@@ -476,6 +476,14 @@
 C ObjC C++ ObjC++ Var(warn_div_by_zero) Init(1) Warning
 Warn about compile-time integer division by zero.
 
+Waccess-specifiers
+C++ ObjC++ Warning Alias(Waccess-specifiers=, 1, 0)
+Warn about redundant access-specifiers.
+
+Waccess-specifiers=
+C++ ObjC++ Var(warn_access_specifiers) Warning Joined RejectNegative UInteger
+Warn about redundant 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-22  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_maybe_warn_access_specifier): New function
	to warn about duplicated access-specifiers.
	(cp_parser_member_specification_opt): Call it.	Set
	current_access_specifier_loc.

Index: gcc/cp/class.c
===================================================================
--- gcc/cp/class.c	(revision 250443)
+++ 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;
@@ -7724,6 +7725,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++;
@@ -7739,6 +7741,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
@@ -7778,6 +7781,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/cp-tree.h
===================================================================
--- gcc/cp/cp-tree.h	(revision 250443)
+++ 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/parser.c
===================================================================
--- gcc/cp/parser.c	(revision 250453)
+++ gcc/cp/parser.c	(working copy)
@@ -23082,6 +23082,69 @@
   return;
 }
 
+/* Warn about redundant access-specifiers.  */
+
+static void
+cp_parser_maybe_warn_access_specifier (cp_parser* parser)
+{
+  cp_token *token = cp_lexer_peek_token (parser->lexer);
+  cp_token *colon_token = cp_lexer_peek_nth_token (parser->lexer, 2);
+  cp_token *next_token = cp_lexer_peek_nth_token (parser->lexer, 3);
+
+  int message_id = 0;
+
+  /* Check for two consecutive access-specifiers.  */
+  if (colon_token->type == CPP_COLON && (next_token->keyword == RID_PUBLIC
+					 || next_token->keyword == RID_PROTECTED
+					 || next_token->keyword == RID_PRIVATE))
+    message_id = 1;
+  /* Check for access-specifiers not changing access.  */
+  else if (current_access_specifier == token->u.value)
+    {
+      if (current_access_specifier_loc != UNKNOWN_LOCATION)
+        message_id = 2;
+      else if (warn_access_specifiers > 1)
+        message_id = 3;
+    }
+
+  if (!message_id)
+    return;
+
+  /* Emit warning.  */
+  gcc_rich_location richloc (token->location);
+  richloc.add_fixit_remove ();
+  if (colon_token->type == CPP_COLON)
+    richloc.add_fixit_remove (colon_token->location);
+
+  switch (message_id)
+    {
+    case 1:
+      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
+			   "redundant %qE access-specifier",
+			   token->u.value);
+      inform (next_token->location, "directly followed by another one here");
+      break;
+
+    case 2:
+      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
+			   "duplicate %qE access-specifier",
+			   token->u.value);
+      inform (current_access_specifier_loc,
+	      "same access-specifier was previously given here");
+      break;
+
+    case 3:
+      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
+			   "access-specifier %qE is redundant within %qs",
+			   token->u.value,
+			   class_key_or_enum_as_string (current_class_type));
+      break;
+
+    default:
+      gcc_unreachable ();
+    }
+}
+
 /* Parse an (optional) member-specification.
 
    member-specification:
@@ -23111,10 +23174,15 @@
 	case RID_PUBLIC:
 	case RID_PROTECTED:
 	case RID_PRIVATE:
+	  /* Warn about redundant access-specifiers.  */
+	  if (warn_access_specifiers)
+	    cp_parser_maybe_warn_access_specifier (parser);
+
 	  /* Consume the access-specifier.  */
 	  cp_lexer_consume_token (parser->lexer);
 	  /* 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-22  Volker Reichelt  <v.reichelt@netcologne.de>

	* g++.dg/warn/Waccess-specifiers-1.C: New test.
	* g++.dg/warn/Waccess-specifiers-2.C: New test.

Index: gcc/testsuite/g++.dg/warn/Waccess-specifiers-1.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Waccess-specifiers-1.C	2017-07-22
+++ gcc/testsuite/g++.dg/warn/Waccess-specifiers-1.C	2017-07-22
@@ -0,0 +1,38 @@
+// { dg-options "-Waccess-specifiers" }
+
+struct A
+{
+    int i;
+  public:         // { dg-message "previously" }
+    int j;
+  public:         // { dg-warning "duplicate 'public' access-specifier" }
+    int k;
+  protected:      // { dg-message "previously" }
+
+    class B
+    {
+      private:    // { dg-message "previously" }
+        int m;
+      private:    // { dg-warning "duplicate 'private' access-specifier" }
+        int n;
+      protected:  // { dg-warning "redundant 'protected' access-specifier" }
+      public:     // { dg-message "directly followed" }
+    };
+
+  protected:      // { dg-warning "duplicate 'protected' access-specifier" }
+    void a();
+  public:
+    void b();
+  private:        // { dg-message "previously" }
+    void c();
+  private:        // { dg-warning "duplicate 'private' access-specifier" }
+    void d();
+  public:
+    void e();
+  protected:      // { dg-message "previously" }
+    void f();
+  protected:      // { dg-warning "duplicate 'protected' access-specifier" }
+                  // { dg-message "previously" "" { target *-*-* } .-1 }
+    void g();
+  protected:      // { dg-warning "duplicate 'protected' access-specifier" }
+};
Index: gcc/testsuite/g++.dg/warn/Waccess-specifiers-2.C
===================================================================
--- gcc/testsuite/g++.dg/warn/Waccess-specifiers-2.C	2017-07-22
+++ gcc/testsuite/g++.dg/warn/Waccess-specifiers-2.C	2017-07-22
@@ -0,0 +1,44 @@
+// { dg-options "-Waccess-specifiers=2 -fdiagnostics-show-caret" }
+
+class A
+{
+  public:  /* { dg-warning "access-specifier" }
+  { dg-begin-multiline-output "" }
+   public:
+   ^~~~~~
+   -------
+  { dg-end-multiline-output "" } */
+
+  private:  /* { dg-message "directly followed" }
+  { dg-begin-multiline-output "" }
+   private:
+   ^~~~~~~
+  { dg-end-multiline-output "" } */
+};
+
+struct B
+{
+  protected:  /* { dg-message "previously" }
+  { dg-begin-multiline-output "" }
+   protected:
+   ^~~~~~~~~
+  { dg-end-multiline-output "" } */
+    B();
+
+  protected  :  /* { dg-warning "access-specifier" }
+  { dg-begin-multiline-output "" }
+   protected  :
+   ^~~~~~~~~
+   ---------  -
+  { dg-end-multiline-output "" } */
+};
+
+class C
+{
+  private:  /* { dg-warning "redundant within" }
+  { dg-begin-multiline-output "" }
+   private:
+   ^~~~~~~
+   --------
+  { dg-end-multiline-output "" } */
+};

Re: [PATCH v2] New C++ warning option '-Waccess-specifiers'

[Eric Gallager]

On 7/23/17, Volker Reichelt <v.reichelt@netcologne.de> wrote:
> Hi again,
>
> here is an updated patch for a new warning about redundant
> access-specifiers. It takes Dave's various comments into account.
>
> The main changes w.r.t. to the previous versions are:
>
> * The warning is now a two-level warning with a slightly shorter name:
>   -Waccess-specifiers=1, -Waccess-specifiers=2
>   with -Waccess-specifiers defaulting to -Waccess-specifiers=1.

Just a more generalized comment as a user, but I don't really like
this trend that new warning options are so often given numeric levels
these days. A warning option with different levels requires special
handling in configure scripts or Makefiles, which is harder than just
toggling different names (i.e. how things work without numeric
levels).

> * The warning now checks for 3 different scenarios, see testcase
>   Waccess-specifiers-2.C below:
>   A) Two consecutive access-specifiers, so that the first one
>      has no effect. (This is new.)
>   B) Two (non-consecutive) equal access-specifiers.
>      (That's what the original patch checked.)
>   C) An access-specifier that does not change the class-type's default.
>      (That's what I only suggested in the original patch.)
>   The first two tests are enabled in level 1, the last in level 2.

Instead of levels, I'd prefer A and B to be in an option like the
original -Wduplicate-access-specifiers, while putting C under a
separate name like -Wdefault-access-specifiers. This way they can be
toggled individually, so for example I can get just warning C without
also getting warnings A and B. Using numeric levels makes that
impossible. But that's just my personal preference for how to separate
them, so feel free to disregard me.

> * The warning is now in a separate function.
> * The fix-it hints now include the colon after the access-specifier.
> * There's a testcase for the fix-it hints.
>
> What's missing from Dave's comments are some examples for invoke.texi.
> I'll work on that once the warning made it into the trunk.
>
> The switch statement in cp_parser_maybe_warn_access_specifier is a little
> bit ugly. It would be nicer to emit the warnings directly next to the
> checks. But then I'd have to write the code for the fix-it hint 3 times.
> With C++11 I'd use a lambda for this, but with C++03 I hope the switch
> is OK.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu.
> OK for trunk?
>
> Regards,
> Volker
>
>
> 2017-07-22  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* doc/invoke.texi (-Waccess-specifiers, -Waccess-specifiers=):
> 	Document new warning options.
>
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi	(revision 250356)
> +++ gcc/doc/invoke.texi	(working copy)
> @@ -259,7 +259,8 @@
>  @xref{Warning Options,,Options to Request or Suppress Warnings}.
>  @gccoptlist{-fsyntax-only  -fmax-errors=@var{n}  -Wpedantic @gol
>  -pedantic-errors @gol
> --w  -Wextra  -Wall  -Waddress  -Waggregate-return  @gol
> +-w  -Wextra  -Wall  -Waccess-specifiers  -Waccess-specifiers=@var{n} @gol
> +-Waddress  -Waggregate-return  @gol
>  -Walloc-zero  -Walloc-size-larger-than=@var{n}
>  -Walloca  -Walloca-larger-than=@var{n} @gol
>  -Wno-aggressive-loop-optimizations  -Warray-bounds  -Warray-bounds=@var{n}
> @gol
> @@ -5254,6 +5255,13 @@
>  Warn about overriding virtual functions that are not marked with the
> override
>  keyword.
>
> +@item -Waccess-specifiers @r{(C++ and Objective-C++ only)}
> +@itemx -Waccess-specifiers=@var{n}
> +@opindex Wno-access-specifiers
> +@opindex Waccess-specifiers
> +Warn when an access-specifier is redundant because it was already given
> +before.
> +
>  @item -Walloc-zero
>  @opindex Wno-alloc-zero
>  @opindex Walloc-zero
> ===================================================================
>
>
> 2017-07-22  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* c.opt (Waccess-specifiers=): New C++ warning flag.
> 	(Waccess-specifiers): New alias.
>
> Index: gcc/c-family/c.opt
> ===================================================================
> --- gcc/c-family/c.opt	(revision 250443)
> +++ gcc/c-family/c.opt	(working copy)
> @@ -476,6 +476,14 @@
>  C ObjC C++ ObjC++ Var(warn_div_by_zero) Init(1) Warning
>  Warn about compile-time integer division by zero.
>
> +Waccess-specifiers
> +C++ ObjC++ Warning Alias(Waccess-specifiers=, 1, 0)
> +Warn about redundant access-specifiers.
> +
> +Waccess-specifiers=
> +C++ ObjC++ Var(warn_access_specifiers) Warning Joined RejectNegative
> UInteger
> +Warn about redundant 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-22  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_maybe_warn_access_specifier): New function
> 	to warn about duplicated access-specifiers.
> 	(cp_parser_member_specification_opt): Call it.	Set
> 	current_access_specifier_loc.
>
> Index: gcc/cp/class.c
> ===================================================================
> --- gcc/cp/class.c	(revision 250443)
> +++ 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;
> @@ -7724,6 +7725,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++;
> @@ -7739,6 +7741,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
> @@ -7778,6 +7781,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/cp-tree.h
> ===================================================================
> --- gcc/cp/cp-tree.h	(revision 250443)
> +++ 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/parser.c
> ===================================================================
> --- gcc/cp/parser.c	(revision 250453)
> +++ gcc/cp/parser.c	(working copy)
> @@ -23082,6 +23082,69 @@
>    return;
>  }
>
> +/* Warn about redundant access-specifiers.  */
> +
> +static void
> +cp_parser_maybe_warn_access_specifier (cp_parser* parser)
> +{
> +  cp_token *token = cp_lexer_peek_token (parser->lexer);
> +  cp_token *colon_token = cp_lexer_peek_nth_token (parser->lexer, 2);
> +  cp_token *next_token = cp_lexer_peek_nth_token (parser->lexer, 3);
> +
> +  int message_id = 0;
> +
> +  /* Check for two consecutive access-specifiers.  */
> +  if (colon_token->type == CPP_COLON && (next_token->keyword == RID_PUBLIC
> +					 || next_token->keyword == RID_PROTECTED
> +					 || next_token->keyword == RID_PRIVATE))
> +    message_id = 1;
> +  /* Check for access-specifiers not changing access.  */
> +  else if (current_access_specifier == token->u.value)
> +    {
> +      if (current_access_specifier_loc != UNKNOWN_LOCATION)
> +        message_id = 2;
> +      else if (warn_access_specifiers > 1)
> +        message_id = 3;
> +    }
> +
> +  if (!message_id)
> +    return;
> +
> +  /* Emit warning.  */
> +  gcc_rich_location richloc (token->location);
> +  richloc.add_fixit_remove ();
> +  if (colon_token->type == CPP_COLON)
> +    richloc.add_fixit_remove (colon_token->location);
> +
> +  switch (message_id)
> +    {
> +    case 1:
> +      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
> +			   "redundant %qE access-specifier",
> +			   token->u.value);
> +      inform (next_token->location, "directly followed by another one
> here");
> +      break;
> +
> +    case 2:
> +      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
> +			   "duplicate %qE access-specifier",
> +			   token->u.value);
> +      inform (current_access_specifier_loc,
> +	      "same access-specifier was previously given here");
> +      break;
> +
> +    case 3:
> +      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
> +			   "access-specifier %qE is redundant within %qs",
> +			   token->u.value,
> +			   class_key_or_enum_as_string (current_class_type));
> +      break;
> +
> +    default:
> +      gcc_unreachable ();
> +    }
> +}
> +
>  /* Parse an (optional) member-specification.
>
>     member-specification:
> @@ -23111,10 +23174,15 @@
>  	case RID_PUBLIC:
>  	case RID_PROTECTED:
>  	case RID_PRIVATE:
> +	  /* Warn about redundant access-specifiers.  */
> +	  if (warn_access_specifiers)
> +	    cp_parser_maybe_warn_access_specifier (parser);
> +
>  	  /* Consume the access-specifier.  */
>  	  cp_lexer_consume_token (parser->lexer);
>  	  /* 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-22  Volker Reichelt  <v.reichelt@netcologne.de>
>
> 	* g++.dg/warn/Waccess-specifiers-1.C: New test.
> 	* g++.dg/warn/Waccess-specifiers-2.C: New test.
>
> Index: gcc/testsuite/g++.dg/warn/Waccess-specifiers-1.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/Waccess-specifiers-1.C	2017-07-22
> +++ gcc/testsuite/g++.dg/warn/Waccess-specifiers-1.C	2017-07-22
> @@ -0,0 +1,38 @@
> +// { dg-options "-Waccess-specifiers" }
> +
> +struct A
> +{
> +    int i;
> +  public:         // { dg-message "previously" }
> +    int j;
> +  public:         // { dg-warning "duplicate 'public' access-specifier" }
> +    int k;
> +  protected:      // { dg-message "previously" }
> +
> +    class B
> +    {
> +      private:    // { dg-message "previously" }
> +        int m;
> +      private:    // { dg-warning "duplicate 'private' access-specifier" }
> +        int n;
> +      protected:  // { dg-warning "redundant 'protected' access-specifier"
> }
> +      public:     // { dg-message "directly followed" }
> +    };
> +
> +  protected:      // { dg-warning "duplicate 'protected' access-specifier"
> }
> +    void a();
> +  public:
> +    void b();
> +  private:        // { dg-message "previously" }
> +    void c();
> +  private:        // { dg-warning "duplicate 'private' access-specifier" }
> +    void d();
> +  public:
> +    void e();
> +  protected:      // { dg-message "previously" }
> +    void f();
> +  protected:      // { dg-warning "duplicate 'protected' access-specifier"
> }
> +                  // { dg-message "previously" "" { target *-*-* } .-1 }
> +    void g();
> +  protected:      // { dg-warning "duplicate 'protected' access-specifier"
> }
> +};
> Index: gcc/testsuite/g++.dg/warn/Waccess-specifiers-2.C
> ===================================================================
> --- gcc/testsuite/g++.dg/warn/Waccess-specifiers-2.C	2017-07-22
> +++ gcc/testsuite/g++.dg/warn/Waccess-specifiers-2.C	2017-07-22
> @@ -0,0 +1,44 @@
> +// { dg-options "-Waccess-specifiers=2 -fdiagnostics-show-caret" }
> +
> +class A
> +{
> +  public:  /* { dg-warning "access-specifier" }
> +  { dg-begin-multiline-output "" }
> +   public:
> +   ^~~~~~
> +   -------
> +  { dg-end-multiline-output "" } */
> +
> +  private:  /* { dg-message "directly followed" }
> +  { dg-begin-multiline-output "" }
> +   private:
> +   ^~~~~~~
> +  { dg-end-multiline-output "" } */
> +};
> +
> +struct B
> +{
> +  protected:  /* { dg-message "previously" }
> +  { dg-begin-multiline-output "" }
> +   protected:
> +   ^~~~~~~~~
> +  { dg-end-multiline-output "" } */
> +    B();
> +
> +  protected  :  /* { dg-warning "access-specifier" }
> +  { dg-begin-multiline-output "" }
> +   protected  :
> +   ^~~~~~~~~
> +   ---------  -
> +  { dg-end-multiline-output "" } */
> +};
> +
> +class C
> +{
> +  private:  /* { dg-warning "redundant within" }
> +  { dg-begin-multiline-output "" }
> +   private:
> +   ^~~~~~~
> +   --------
> +  { dg-end-multiline-output "" } */
> +};
>
>

Re: [PATCH v2] New C++ warning option '-Waccess-specifiers'

[Volker Reichelt]

On 23 Jul, Eric Gallager wrote:
> On 7/23/17, Volker Reichelt <v.reichelt@netcologne.de> wrote:
>> Hi again,
>>
>> here is an updated patch for a new warning about redundant
>> access-specifiers. It takes Dave's various comments into account.
>>
>> The main changes w.r.t. to the previous versions are:
>>
>> * The warning is now a two-level warning with a slightly shorter name:
>>   -Waccess-specifiers=1, -Waccess-specifiers=2
>>   with -Waccess-specifiers defaulting to -Waccess-specifiers=1.
> 
> Just a more generalized comment as a user, but I don't really like
> this trend that new warning options are so often given numeric levels
> these days. A warning option with different levels requires special
> handling in configure scripts or Makefiles, which is harder than just
> toggling different names (i.e. how things work without numeric
> levels).

Fair point.

>> * The warning now checks for 3 different scenarios, see testcase
>>   Waccess-specifiers-2.C below:
>>   A) Two consecutive access-specifiers, so that the first one
>>      has no effect. (This is new.)
>>   B) Two (non-consecutive) equal access-specifiers.
>>      (That's what the original patch checked.)
>>   C) An access-specifier that does not change the class-type's default.
>>      (That's what I only suggested in the original patch.)
>>   The first two tests are enabled in level 1, the last in level 2.
> 
> Instead of levels, I'd prefer A and B to be in an option like the
> original -Wduplicate-access-specifiers, while putting C under a
> separate name like -Wdefault-access-specifiers. This way they can be
> toggled individually, so for example I can get just warning C without
> also getting warnings A and B. Using numeric levels makes that
> impossible. But that's just my personal preference for how to separate
> them, so feel free to disregard me.

What I like about the numeric levels is that they are easier to
remember. You don't have to look up different names.
OTOH, toggling on C without A+B might really make sense here.

But I'd like to get some more opinions before I change that.

Regards,
Volker

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

[Martin Sebor]

On 07/23/2017 02:42 PM, Volker Reichelt wrote:
> On 21 Jul, Martin Sebor wrote:
>> On 07/20/2017 10:35 AM, 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;
>>>   };
>>
>> I'm very fond of warnings that help find real bugs, or even that
>> provide an opportunity to review code for potential problems or
>> inefficiencies and suggest a possibility to improve it in some
>> way (make it clearer, or easier for humans or compilers to find
>> real bugs in, or faster, etc.), even if the code isn't strictly
>> incorrect.
>>
>> In this case I'm having trouble coming up with an example where
>> the warning would have this effect.  What do you have in mind?
>> (Duplicate access specifiers tend to crop up in large classes
>> and/or as a result of preprocessor conditionals.)
>
> This warning fights the "tend to crop up" effect that clutters the
> code. After some time these redundant access specifiers creep in
> and make your code harder to read. If I see something like
>
>   ...
>     void foo() const;
>
>   private:
>     void bar();
>   ...
>
> on the screen I tend to think that 'foo' has a different access
> level than bar. If that's not the case because the access-specifier
> is redundant, then that's just confusing and distracting.
> The warning helps to maintain readability of your code.
>
> The benefit might not be big, but the warning comes at relatively
> low cost. It passes a location around through the class stack and
> checks less than a handful of tokens.
>
> My personal motivation to implement this warning was the fact that
> I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT
> mechanism to the new function pointer syntax of Qt5. In the old
> version you had to mark slots in the following fashion:
>
>   public slots:
>     void foo();
>     void bar();
>
> But now you can use any function making the 'slots' macro obsolete.
> Therefore I ended up with hundreds of redundant access-specifiers
> which this warning helped to clean up. Doing this sort of thing in the
> compiler with a proper parser is way safer than to write some script
> to achieve this.

Okay, thanks for clarifying that.  I think what's distracting to
one could be helpful to another.  For example, it's not uncommon
for classes with many members to use redundant access specifiers
to group blocks of related declarations.  Or, in a class with many
lines of comments (e.g., Doxygen), repeating the access specifier
every so often could be seen as helpful because otherwise there
would be no way to tell what its access is without scrolling up
or down.  It's debatable what approach to dealing with this is
best.  Java, for instance, requires every non-public member to
be declared with its own access specifier.  Some other languages
(I think D) do as well.  An argument could be made that that's
a far clearer style than using the specifier only when changing
access.  It seems to me that the most suitable approach will be
different from one project to another, if not from one person to
another.  A diagnostic that recommends a particular style (or that
helps with a specific kind of a project like the one you did for
Qt) might be a good candidate for a plugin, but enshrining any
one style (or a solution to a project-specific problem) in GCC
as a general-purpose warning doesn't seem appropriate or in line
with the definition of warnings in GCC:

   constructions that are not inherently erroneous but that are
   risky or suggest there may have been an error

Martin

PS There are other redundancies that some might say unnecessarily
clutter code.  For instance, declaring a symbol static in
an unnamed namespace, or explicitly declaring a member function
inline that's also defined within the body of a class, or
explicitly declaring a function virtual that overrides one
declared in a base class.  None of these is diagnosed, and I'd
say for good reason: they are all benign and do not suggest any
sort of a coding mistake or present an opportunity for improvement.
In fact, warning for some of them (especially the virtual function)
might even be detrimental

> Btw, the SonarAnalyzer also provides this warning to clean up code:
>
> https://sonarcloud.io/organizations/default/rules#rule_key=cpp%3AS3539

Thanks, that's an interesting data point.

Martin

>
>> Btw., there are examples of poor coding practices where I can
>> imagine a warning focused on access specifiers being helpful.
>> For instance, a class whose member functions maintain non-trivial
>> invariants among its data members should declare the data private
>> to prevent clients from inadvertently breaking those invariants.
>> A specific example might be a container class (like string or
>> vector) that owns the block of memory it allocates.  A warning
>> that detected when such members are publicly accessible could
>> help improve encapsulation.  I suspect this would be challenging
>> to implement and would likely require some non-trivial analysis
>> in the middle end.
>
> That's way beyond the scope of what my warning is trying to achieve.
>
>> Another example is a class that defines an inaccessible member
>> that isn't used (e.g., class A { int i; }; that Clang detects
>> with -Wunused-private-field; or class A { void f () { } };).
>> I would expect this to be doable in the front end.
>
> That's indeed a nice warning, too.
>
>> Martin
>
> Regards,
> Volker
>

Re: [PATCH v2] New C++ warning option '-Waccess-specifiers'

[Franz Sirl]

Am 2017-07-24 um 00:19 schrieb Volker Reichelt:
> On 23 Jul, Eric Gallager wrote:
>> On 7/23/17, Volker Reichelt <v.reichelt@netcologne.de> wrote:
>>> Hi again,
>>>
>>> here is an updated patch for a new warning about redundant
>>> access-specifiers. It takes Dave's various comments into account.
>>>
>>> The main changes w.r.t. to the previous versions are:
>>>
>>> * The warning is now a two-level warning with a slightly shorter name:
>>>    -Waccess-specifiers=1, -Waccess-specifiers=2
>>>    with -Waccess-specifiers defaulting to -Waccess-specifiers=1.
>>
>> Just a more generalized comment as a user, but I don't really like
>> this trend that new warning options are so often given numeric levels
>> these days. A warning option with different levels requires special
>> handling in configure scripts or Makefiles, which is harder than just
>> toggling different names (i.e. how things work without numeric
>> levels).
> 
> Fair point.

Another point is the handling of -Werror=. AFAIK it would be impossible 
right now to have "-Werror=access-specifiers=1 -Waccess-specifiers=2", 
with a combined meaning of "error for level 1 + warning for level 2".

Actually, are the intended semantics for the existing cases (eg. 
-Warray-bounds=) vs. -Werror= even documented somewhere?

Franz

Re: [PATCH v2] New C++ warning option '-Waccess-specifiers'

[Marek Polacek]

On Sun, Jul 23, 2017 at 11:22:14PM +0200, Volker Reichelt wrote:
[...]

Not sure if the warning is too useful, but in any case...

> +  /* Emit warning.  */
> +  gcc_rich_location richloc (token->location);
> +  richloc.add_fixit_remove ();
> +  if (colon_token->type == CPP_COLON)
> +    richloc.add_fixit_remove (colon_token->location);
> +
> +  switch (message_id)
> +    {
> +    case 1:
> +      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
> +			   "redundant %qE access-specifier",
> +			   token->u.value);
> +      inform (next_token->location, "directly followed by another one here");
> +      break;
> +
> +    case 2:
> +      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
> +			   "duplicate %qE access-specifier",
> +			   token->u.value);
> +      inform (current_access_specifier_loc,
> +	      "same access-specifier was previously given here");
> +      break;

...you should only call inform if warning_at_rich_loc returned true.

	Marek

Re: [PATCH v2] New C++ warning option '-Waccess-specifiers'

[Volker Reichelt]

On 25 Jul, Marek Polacek wrote:
> On Sun, Jul 23, 2017 at 11:22:14PM +0200, Volker Reichelt wrote:
> [...]
> 
> Not sure if the warning is too useful, but in any case...
> 
>> +  /* Emit warning.  */
>> +  gcc_rich_location richloc (token->location);
>> +  richloc.add_fixit_remove ();
>> +  if (colon_token->type == CPP_COLON)
>> +    richloc.add_fixit_remove (colon_token->location);
>> +
>> +  switch (message_id)
>> +    {
>> +    case 1:
>> +      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
>> +			   "redundant %qE access-specifier",
>> +			   token->u.value);
>> +      inform (next_token->location, "directly followed by another one here");
>> +      break;
>> +
>> +    case 2:
>> +      warning_at_rich_loc (&richloc, OPT_Waccess_specifiers_,
>> +			   "duplicate %qE access-specifier",
>> +			   token->u.value);
>> +      inform (current_access_specifier_loc,
>> +	      "same access-specifier was previously given here");
>> +      break;
> 
> ...you should only call inform if warning_at_rich_loc returned true.
> 
> 	Marek

I also noticed yesterday that the inform calls weren't suppressed
together with the warningss in system headers. I was thinking about
adding an in_system_header_at check, but your your suggestion is much
nicer.

Thanks,
Volker

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

[Volker Reichelt]

On 23 Jul, Martin Sebor wrote:
> On 07/23/2017 02:42 PM, Volker Reichelt wrote:
>> On 21 Jul, Martin Sebor wrote:
>>> On 07/20/2017 10:35 AM, 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;
>>>>   };
>>>
>>> I'm very fond of warnings that help find real bugs, or even that
>>> provide an opportunity to review code for potential problems or
>>> inefficiencies and suggest a possibility to improve it in some
>>> way (make it clearer, or easier for humans or compilers to find
>>> real bugs in, or faster, etc.), even if the code isn't strictly
>>> incorrect.
>>>
>>> In this case I'm having trouble coming up with an example where
>>> the warning would have this effect.  What do you have in mind?
>>> (Duplicate access specifiers tend to crop up in large classes
>>> and/or as a result of preprocessor conditionals.)
>>
>> This warning fights the "tend to crop up" effect that clutters the
>> code. After some time these redundant access specifiers creep in
>> and make your code harder to read. If I see something like
>>
>>   ...
>>     void foo() const;
>>
>>   private:
>>     void bar();
>>   ...
>>
>> on the screen I tend to think that 'foo' has a different access
>> level than bar. If that's not the case because the access-specifier
>> is redundant, then that's just confusing and distracting.
>> The warning helps to maintain readability of your code.
>>
>> The benefit might not be big, but the warning comes at relatively
>> low cost. It passes a location around through the class stack and
>> checks less than a handful of tokens.
>>
>> My personal motivation to implement this warning was the fact that
>> I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT
>> mechanism to the new function pointer syntax of Qt5. In the old
>> version you had to mark slots in the following fashion:
>>
>>   public slots:
>>     void foo();
>>     void bar();
>>
>> But now you can use any function making the 'slots' macro obsolete.
>> Therefore I ended up with hundreds of redundant access-specifiers
>> which this warning helped to clean up. Doing this sort of thing in the
>> compiler with a proper parser is way safer than to write some script
>> to achieve this.
> 
> Okay, thanks for clarifying that.  I think what's distracting to
> one could be helpful to another.  For example, it's not uncommon
> for classes with many members to use redundant access specifiers
> to group blocks of related declarations.  Or, in a class with many
> lines of comments (e.g., Doxygen), repeating the access specifier
> every so often could be seen as helpful because otherwise there
> would be no way to tell what its access is without scrolling up
> or down.  It's debatable what approach to dealing with this is
> best.  Java, for instance, requires every non-public member to
> be declared with its own access specifier.  Some other languages
> (I think D) do as well.  An argument could be made that that's
> a far clearer style than using the specifier only when changing
> access.  It seems to me that the most suitable approach will be
> different from one project to another, if not from one person to
> another.  A diagnostic that recommends a particular style (or that
> helps with a specific kind of a project like the one you did for
> Qt) might be a good candidate for a plugin, but enshrining any
> one style (or a solution to a project-specific problem) in GCC
> as a general-purpose warning doesn't seem appropriate or in line
> with the definition of warnings in GCC:
> 
>    constructions that are not inherently erroneous but that are
>    risky or suggest there may have been an error
> 
> Martin
> 
> PS There are other redundancies that some might say unnecessarily
> clutter code.  For instance, declaring a symbol static in
> an unnamed namespace, or explicitly declaring a member function
> inline that's also defined within the body of a class, or
> explicitly declaring a function virtual that overrides one
> declared in a base class.  None of these is diagnosed, and I'd
> say for good reason: they are all benign and do not suggest any
> sort of a coding mistake or present an opportunity for improvement.
> In fact, warning for some of them (especially the virtual function)
> might even be detrimental
> 
>> Btw, the SonarAnalyzer also provides this warning to clean up code:
>>
>> https://sonarcloud.io/organizations/default/rules#rule_key=cpp%3AS3539
> 
> Thanks, that's an interesting data point.
> 
> Martin

Maybe the whole thing of diagnostics that refer more to style instead of
correctness or potential bugs shoould be discussed (independent of my
patch) by a broader audience.

There are several tools out there like clang-tidy, SonarAnalyzer etc.
that not only check for bugs but also flag stylistic issues. For GCC I
could imagine a set of such wanings prefixed by -Wstyle- or something
similar to make the intent clear. I'll post something on the main
mailing list in the next days.

Regards,
Volker

Re: [PATCH v2] New C++ warning option '-Waccess-specifiers'

[Eric Gallager]

On 7/24/17, Franz Sirl <Franz.Sirl-kernel@lauterbach.com> wrote:
> Am 2017-07-24 um 00:19 schrieb Volker Reichelt:
>> On 23 Jul, Eric Gallager wrote:
>>> On 7/23/17, Volker Reichelt <v.reichelt@netcologne.de> wrote:
>>>> Hi again,
>>>>
>>>> here is an updated patch for a new warning about redundant
>>>> access-specifiers. It takes Dave's various comments into account.
>>>>
>>>> The main changes w.r.t. to the previous versions are:
>>>>
>>>> * The warning is now a two-level warning with a slightly shorter name:
>>>>    -Waccess-specifiers=1, -Waccess-specifiers=2
>>>>    with -Waccess-specifiers defaulting to -Waccess-specifiers=1.
>>>
>>> Just a more generalized comment as a user, but I don't really like
>>> this trend that new warning options are so often given numeric levels
>>> these days. A warning option with different levels requires special
>>> handling in configure scripts or Makefiles, which is harder than just
>>> toggling different names (i.e. how things work without numeric
>>> levels).
>>
>> Fair point.
>
> Another point is the handling of -Werror=. AFAIK it would be impossible
> right now to have "-Werror=access-specifiers=1 -Waccess-specifiers=2",
> with a combined meaning of "error for level 1 + warning for level 2".
>
> Actually, are the intended semantics for the existing cases (eg.
> -Warray-bounds=) vs. -Werror= even documented somewhere?
>
> Franz
>

Not exactly documentation, but there are bugs open about it:
56048: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=56048
68845: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68845 (which I see
is yours, Franz)
Might be worth having someone take a look at them.

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

[Richard Sandiford]

Martin Sebor <msebor@gmail.com> writes:
> On 07/23/2017 02:42 PM, Volker Reichelt wrote:
>> On 21 Jul, Martin Sebor wrote:
>>> On 07/20/2017 10:35 AM, 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;
>>>>   };
>>>
>>> I'm very fond of warnings that help find real bugs, or even that
>>> provide an opportunity to review code for potential problems or
>>> inefficiencies and suggest a possibility to improve it in some
>>> way (make it clearer, or easier for humans or compilers to find
>>> real bugs in, or faster, etc.), even if the code isn't strictly
>>> incorrect.
>>>
>>> In this case I'm having trouble coming up with an example where
>>> the warning would have this effect.  What do you have in mind?
>>> (Duplicate access specifiers tend to crop up in large classes
>>> and/or as a result of preprocessor conditionals.)
>>
>> This warning fights the "tend to crop up" effect that clutters the
>> code. After some time these redundant access specifiers creep in
>> and make your code harder to read. If I see something like
>>
>>   ...
>>     void foo() const;
>>
>>   private:
>>     void bar();
>>   ...
>>
>> on the screen I tend to think that 'foo' has a different access
>> level than bar. If that's not the case because the access-specifier
>> is redundant, then that's just confusing and distracting.
>> The warning helps to maintain readability of your code.
>>
>> The benefit might not be big, but the warning comes at relatively
>> low cost. It passes a location around through the class stack and
>> checks less than a handful of tokens.
>>
>> My personal motivation to implement this warning was the fact that
>> I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT
>> mechanism to the new function pointer syntax of Qt5. In the old
>> version you had to mark slots in the following fashion:
>>
>>   public slots:
>>     void foo();
>>     void bar();
>>
>> But now you can use any function making the 'slots' macro obsolete.
>> Therefore I ended up with hundreds of redundant access-specifiers
>> which this warning helped to clean up. Doing this sort of thing in the
>> compiler with a proper parser is way safer than to write some script
>> to achieve this.
>
> Okay, thanks for clarifying that.  I think what's distracting to
> one could be helpful to another.  For example, it's not uncommon
> for classes with many members to use redundant access specifiers
> to group blocks of related declarations.  Or, in a class with many
> lines of comments (e.g., Doxygen), repeating the access specifier
> every so often could be seen as helpful because otherwise there
> would be no way to tell what its access is without scrolling up
> or down.  It's debatable what approach to dealing with this is
> best.  Java, for instance, requires every non-public member to
> be declared with its own access specifier.  Some other languages
> (I think D) do as well.  An argument could be made that that's
> a far clearer style than using the specifier only when changing
> access.  It seems to me that the most suitable approach will be
> different from one project to another, if not from one person to
> another.  A diagnostic that recommends a particular style (or that
> helps with a specific kind of a project like the one you did for
> Qt) might be a good candidate for a plugin, but enshrining any
> one style (or a solution to a project-specific problem) in GCC
> as a general-purpose warning doesn't seem appropriate or in line
> with the definition of warnings in GCC:
>
>    constructions that are not inherently erroneous but that are
>    risky or suggest there may have been an error

I think there are some circumstances in which the warning would count,
especially if you're working to a coding convention that requires all
public members followed by all protected members followed by all private
members.  Having a duplicated specifier in that context might then
indicate that you've got a cut-&-paste error.

I think both that scenario and the ones Volker gave are enough
justification for the warning to be useful, but not enough for
including it in Wall or Wextra (which isn't being proposed).

> PS There are other redundancies that some might say unnecessarily
> clutter code.  For instance, declaring a symbol static in
> an unnamed namespace, or explicitly declaring a member function
> inline that's also defined within the body of a class, or
> explicitly declaring a function virtual that overrides one
> declared in a base class.  None of these is diagnosed, and I'd
> say for good reason: they are all benign and do not suggest any
> sort of a coding mistake or present an opportunity for improvement.
> In fact, warning for some of them (especially the virtual function)
> might even be detrimental

Actually, I've wanted one for the "static in an anonymous namespace"
in the past :-)

But I think you could also say the same about things like
-Wmissing-declarations.  That warns about something as simple as:

void foo(void) {}

which I think is even harder to justify as being inherently suspicious.
But in the ANSI C days, this was very useful in enforcing GCC's own coding
convention, and so was enabled during bootstrap.

Thanks,
Richard

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

[Martin Sebor]

On 07/27/2017 01:13 PM, Richard Sandiford wrote:
> Martin Sebor <msebor@gmail.com> writes:
>> On 07/23/2017 02:42 PM, Volker Reichelt wrote:
>>> On 21 Jul, Martin Sebor wrote:
>>>> On 07/20/2017 10:35 AM, 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;
>>>>>   };
>>>>
>>>> I'm very fond of warnings that help find real bugs, or even that
>>>> provide an opportunity to review code for potential problems or
>>>> inefficiencies and suggest a possibility to improve it in some
>>>> way (make it clearer, or easier for humans or compilers to find
>>>> real bugs in, or faster, etc.), even if the code isn't strictly
>>>> incorrect.
>>>>
>>>> In this case I'm having trouble coming up with an example where
>>>> the warning would have this effect.  What do you have in mind?
>>>> (Duplicate access specifiers tend to crop up in large classes
>>>> and/or as a result of preprocessor conditionals.)
>>>
>>> This warning fights the "tend to crop up" effect that clutters the
>>> code. After some time these redundant access specifiers creep in
>>> and make your code harder to read. If I see something like
>>>
>>>   ...
>>>     void foo() const;
>>>
>>>   private:
>>>     void bar();
>>>   ...
>>>
>>> on the screen I tend to think that 'foo' has a different access
>>> level than bar. If that's not the case because the access-specifier
>>> is redundant, then that's just confusing and distracting.
>>> The warning helps to maintain readability of your code.
>>>
>>> The benefit might not be big, but the warning comes at relatively
>>> low cost. It passes a location around through the class stack and
>>> checks less than a handful of tokens.
>>>
>>> My personal motivation to implement this warning was the fact that
>>> I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT
>>> mechanism to the new function pointer syntax of Qt5. In the old
>>> version you had to mark slots in the following fashion:
>>>
>>>   public slots:
>>>     void foo();
>>>     void bar();
>>>
>>> But now you can use any function making the 'slots' macro obsolete.
>>> Therefore I ended up with hundreds of redundant access-specifiers
>>> which this warning helped to clean up. Doing this sort of thing in the
>>> compiler with a proper parser is way safer than to write some script
>>> to achieve this.
>>
>> Okay, thanks for clarifying that.  I think what's distracting to
>> one could be helpful to another.  For example, it's not uncommon
>> for classes with many members to use redundant access specifiers
>> to group blocks of related declarations.  Or, in a class with many
>> lines of comments (e.g., Doxygen), repeating the access specifier
>> every so often could be seen as helpful because otherwise there
>> would be no way to tell what its access is without scrolling up
>> or down.  It's debatable what approach to dealing with this is
>> best.  Java, for instance, requires every non-public member to
>> be declared with its own access specifier.  Some other languages
>> (I think D) do as well.  An argument could be made that that's
>> a far clearer style than using the specifier only when changing
>> access.  It seems to me that the most suitable approach will be
>> different from one project to another, if not from one person to
>> another.  A diagnostic that recommends a particular style (or that
>> helps with a specific kind of a project like the one you did for
>> Qt) might be a good candidate for a plugin, but enshrining any
>> one style (or a solution to a project-specific problem) in GCC
>> as a general-purpose warning doesn't seem appropriate or in line
>> with the definition of warnings in GCC:
>>
>>    constructions that are not inherently erroneous but that are
>>    risky or suggest there may have been an error
>
> I think there are some circumstances in which the warning would count,
> especially if you're working to a coding convention that requires all
> public members followed by all protected members followed by all private
> members.  Having a duplicated specifier in that context might then
> indicate that you've got a cut-&-paste error.

Perhaps.  It sounds like a stretch to me.  But my argument isn't
that no styles exist that the warning might work for but rather
that it tries to enforce a unique (and IMO, rare) style to the
exclusion some common and arguably more useful ones.  It's a wide
spread convention to use an access specifier to introduce a block
of related declarations.  For example:

   class X {
     ...
     public:   // public types
     ...
     public:   // public functions
     ...
     private:  // private functions
     ...
     private:  // private data
     ...
   };

Although this is a reasonable convention I think it would be fair
to consider a warning that diagnosed it if it had a good chance
of finding real bugs in some other code.  But this is not the case
here (nor was the purpose of the warning to find such bugs).

> I think both that scenario and the ones Volker gave are enough
> justification for the warning to be useful, but not enough for
> including it in Wall or Wextra (which isn't being proposed).
>
>> PS There are other redundancies that some might say unnecessarily
>> clutter code.  For instance, declaring a symbol static in
>> an unnamed namespace, or explicitly declaring a member function
>> inline that's also defined within the body of a class, or
>> explicitly declaring a function virtual that overrides one
>> declared in a base class.  None of these is diagnosed, and I'd
>> say for good reason: they are all benign and do not suggest any
>> sort of a coding mistake or present an opportunity for improvement.
>> In fact, warning for some of them (especially the virtual function)
>> might even be detrimental
>
> Actually, I've wanted one for the "static in an anonymous namespace"
> in the past :-)

Out of curiosity, why?

But again, I'm not arguing that there is no use case for finding
these patterns.  Just that they are benign and not suggestive of
bugs or opportunities for improvements, and so not appropriate
for inclusion in the form of warnings whose purpose is to point
out likely coding mistakes that could be the source of bugs.

> But I think you could also say the same about things like
> -Wmissing-declarations.  That warns about something as simple as:
>
> void foo(void) {}
>
> which I think is even harder to justify as being inherently suspicious.
> But in the ANSI C days, this was very useful in enforcing GCC's own coding
> convention, and so was enabled during bootstrap.

Defining a function that's incompatible with its declaration in
another translation unit is a common mistake in C (and undefined
behavior).  Using the function leads to hard to debug problems.
Coding standard rules have been written (such as rule DCL40-C of
the CERT Secure Coding Standard) and static analysis tools have
been developed to detect this problem (see the Automated Detection
section in the CERT rule).  Many compilers implement this warning
for this reason.

Martin