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