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

[Volker Reichelt <v dot reichelt at netcologne dot de>]

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" }
+};