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