On 7/23/17, Volker Reichelt <v.reich...@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.reich...@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.reich...@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.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_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.reich...@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 "" } */
> +};
>
>
