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 <[email protected]>
* 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 <[email protected]>
* 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 <[email protected]>
* 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 <[email protected]>
* 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 "" } */
+};
