| Message ID | tkrat.2284ed76aa47e2e7@netcologne.de |
|---|---|
| State | New |
| Headers | show |
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.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. 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.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 (); 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.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" } > +}; 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
On Fri, 2017-07-21 at 11:56 -0400, David Malcolm wrote: > 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. Maybe combine the two ideas, and call it -Wredundant-access-specifiers ? Or just "-Waccess-specifiers" ? [...snip...] Dave
On 21 Jul, David Malcolm wrote: > On Fri, 2017-07-21 at 11:56 -0400, David Malcolm wrote: >> 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. > > Maybe combine the two ideas, and call it > -Wredundant-access-specifiers > ? > > Or just "-Waccess-specifiers" ? > > [...snip...] > > Dave Thanks for having a look at this! My favorite version would be to use '-Waccess-specifiers=1' for the original warning and '-Waccess-specifiers=2' for the stricter version that also checks against the class-type default. We could then let '-Waccess-specifiers' default to any of those two. I'm afraid that level 2 will fire way too often for legacy code (and there might be coding conventions that require an access specifier at the beginning of a class/struct). So I'd vote for level 1 as default. The last argument is also the reason why I'd like to see a two-lveel warning instead of just different error messages (although these are very helpful for seeing what's really goiing on with your code). What do you think? Regards, Volker
On 07/20/2017 10:35 AM, 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; > }; I'm very fond of warnings that help find real bugs, or even that provide an opportunity to review code for potential problems or inefficiencies and suggest a possibility to improve it in some way (make it clearer, or easier for humans or compilers to find real bugs in, or faster, etc.), even if the code isn't strictly incorrect. In this case I'm having trouble coming up with an example where the warning would have this effect. What do you have in mind? (Duplicate access specifiers tend to crop up in large classes and/or as a result of preprocessor conditionals.) Btw., there are examples of poor coding practices where I can imagine a warning focused on access specifiers being helpful. For instance, a class whose member functions maintain non-trivial invariants among its data members should declare the data private to prevent clients from inadvertently breaking those invariants. A specific example might be a container class (like string or vector) that owns the block of memory it allocates. A warning that detected when such members are publicly accessible could help improve encapsulation. I suspect this would be challenging to implement and would likely require some non-trivial analysis in the middle end. Another example is a class that defines an inaccessible member that isn't used (e.g., class A { int i; }; that Clang detects with -Wunused-private-field; or class A { void f () { } };). I would expect this to be doable in the front end. Martin
On 21 Jul, David Malcolm wrote: > 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. That's what also can be seen in our libstdc++ to some extent. The other half of the warnings indicate redundant access-specifiers. It's up to the user to keep those duplicate access-specifiers as subdividers or to use something else (like comments) to do that and to switch on the warning for her/his code. Because the subdivider usage seems to be relatively common, I don't want to enable the warning by -Wall or -Wextra. > 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.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. > > Presumably this should be marked as C++-specific. Oops, good point! > I think it's best to give an example for any warning, though we don't > do that currently. Sounds reasonable. Especially because 'access-specifier' is a very technical term that is not linked to 'public/protected/private' by everybody. [...snip...] >> 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. I did not do this because I did not want to reorder the code. OTOH just moving the cp_parser_require line a little bit up should not be a big deal for better diagnostics. > 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.reichelt@netcologne.de> >> >> * g++.dg/warn/Wduplicated-access-specifiers-1.C: New test. {...snip...] > 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). I actually did try that, but dejagnu somehow gets confused. To me it looks like the reason for this is that both multi-line messages are so similar. I'll give it a second try, though. > Hope this is constructive. Yes, it is! > Dave
On Fri, 2017-07-21 at 19:58 +0200, Volker Reichelt wrote: > On 21 Jul, David Malcolm wrote: > > 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. > > That's what also can be seen in our libstdc++ to some extent. > The other half of the warnings indicate redundant access-specifiers. > > It's up to the user to keep those duplicate access-specifiers as > subdividers or to use something else (like comments) to do that > and to switch on the warning for her/his code. > Because the subdivider usage seems to be relatively common, > I don't want to enable the warning by -Wall or -Wextra. > > > 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.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. > > > > Presumably this should be marked as C++-specific. > > Oops, good point! > > > I think it's best to give an example for any warning, though we > don't > > do that currently. > > Sounds reasonable. Especially because 'access-specifier' is a very > technical term that is not linked to 'public/protected/private' > by everybody. > > [...snip...] > > >> 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. > > I did not do this because I did not want to reorder the code. > OTOH just moving the cp_parser_require line a little bit up > should not be a big deal for better diagnostics. > > > 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.reichelt@netcologne.de> > >> > >> * g++.dg/warn/Wduplicated-access-specifiers-1.C: New test. > > {...snip...] > > > 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). > > I actually did try that, but dejagnu somehow gets confused. > To me it looks like the reason for this is that both multi-line > messages > are so similar. I'll give it a second try, though. I'm not sure what you mean by "both" multi-line messages; shouldn't there be just one multi-line message for the fix-it hint. Isn't this like e.g. g++.dg/cpp0x/auto1.C ? (an example of a testcase that verifies a deletion fix-it hint) Dave
On 21 Jul, Martin Sebor wrote: > On 07/20/2017 10:35 AM, 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; >> }; > > I'm very fond of warnings that help find real bugs, or even that > provide an opportunity to review code for potential problems or > inefficiencies and suggest a possibility to improve it in some > way (make it clearer, or easier for humans or compilers to find > real bugs in, or faster, etc.), even if the code isn't strictly > incorrect. > > In this case I'm having trouble coming up with an example where > the warning would have this effect. What do you have in mind? > (Duplicate access specifiers tend to crop up in large classes > and/or as a result of preprocessor conditionals.) This warning fights the "tend to crop up" effect that clutters the code. After some time these redundant access specifiers creep in and make your code harder to read. If I see something like ... void foo() const; private: void bar(); ... on the screen I tend to think that 'foo' has a different access level than bar. If that's not the case because the access-specifier is redundant, then that's just confusing and distracting. The warning helps to maintain readability of your code. The benefit might not be big, but the warning comes at relatively low cost. It passes a location around through the class stack and checks less than a handful of tokens. My personal motivation to implement this warning was the fact that I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT mechanism to the new function pointer syntax of Qt5. In the old version you had to mark slots in the following fashion: public slots: void foo(); void bar(); But now you can use any function making the 'slots' macro obsolete. Therefore I ended up with hundreds of redundant access-specifiers which this warning helped to clean up. Doing this sort of thing in the compiler with a proper parser is way safer than to write some script to achieve this. Btw, the SonarAnalyzer also provides this warning to clean up code: https://sonarcloud.io/organizations/default/rules#rule_key=cpp%3AS3539 > Btw., there are examples of poor coding practices where I can > imagine a warning focused on access specifiers being helpful. > For instance, a class whose member functions maintain non-trivial > invariants among its data members should declare the data private > to prevent clients from inadvertently breaking those invariants. > A specific example might be a container class (like string or > vector) that owns the block of memory it allocates. A warning > that detected when such members are publicly accessible could > help improve encapsulation. I suspect this would be challenging > to implement and would likely require some non-trivial analysis > in the middle end. That's way beyond the scope of what my warning is trying to achieve. > Another example is a class that defines an inaccessible member > that isn't used (e.g., class A { int i; }; that Clang detects > with -Wunused-private-field; or class A { void f () { } };). > I would expect this to be doable in the front end. That's indeed a nice warning, too. > Martin Regards, Volker
On 21 Jul, David Malcolm wrote: > On Fri, 2017-07-21 at 19:58 +0200, Volker Reichelt wrote: >> On 21 Jul, David Malcolm wrote: >> > 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: >> >> ^~~~~~~ {...snip...] >> > 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). >> >> I actually did try that, but dejagnu somehow gets confused. >> To me it looks like the reason for this is that both multi-line >> messages >> are so similar. I'll give it a second try, though. > > I'm not sure what you mean by "both" multi-line messages; shouldn't > there be just one multi-line message for the fix-it hint. > > Isn't this like e.g. g++.dg/cpp0x/auto1.C ? (an example of a testcase > that verifies a deletion fix-it hint) > > Dave There are two multi-line messages. One for the warning and one for the note, see the example above the "[...snip...]". The message after the note consists of the first two lines of the message after the warning. This seems to confuse dejagnu. However, I managed to work around this problem. I'll post an updated patch soon. Regards, Volker
On 07/23/2017 02:42 PM, Volker Reichelt wrote: > On 21 Jul, Martin Sebor wrote: >> On 07/20/2017 10:35 AM, 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; >>> }; >> >> I'm very fond of warnings that help find real bugs, or even that >> provide an opportunity to review code for potential problems or >> inefficiencies and suggest a possibility to improve it in some >> way (make it clearer, or easier for humans or compilers to find >> real bugs in, or faster, etc.), even if the code isn't strictly >> incorrect. >> >> In this case I'm having trouble coming up with an example where >> the warning would have this effect. What do you have in mind? >> (Duplicate access specifiers tend to crop up in large classes >> and/or as a result of preprocessor conditionals.) > > This warning fights the "tend to crop up" effect that clutters the > code. After some time these redundant access specifiers creep in > and make your code harder to read. If I see something like > > ... > void foo() const; > > private: > void bar(); > ... > > on the screen I tend to think that 'foo' has a different access > level than bar. If that's not the case because the access-specifier > is redundant, then that's just confusing and distracting. > The warning helps to maintain readability of your code. > > The benefit might not be big, but the warning comes at relatively > low cost. It passes a location around through the class stack and > checks less than a handful of tokens. > > My personal motivation to implement this warning was the fact that > I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT > mechanism to the new function pointer syntax of Qt5. In the old > version you had to mark slots in the following fashion: > > public slots: > void foo(); > void bar(); > > But now you can use any function making the 'slots' macro obsolete. > Therefore I ended up with hundreds of redundant access-specifiers > which this warning helped to clean up. Doing this sort of thing in the > compiler with a proper parser is way safer than to write some script > to achieve this. Okay, thanks for clarifying that. I think what's distracting to one could be helpful to another. For example, it's not uncommon for classes with many members to use redundant access specifiers to group blocks of related declarations. Or, in a class with many lines of comments (e.g., Doxygen), repeating the access specifier every so often could be seen as helpful because otherwise there would be no way to tell what its access is without scrolling up or down. It's debatable what approach to dealing with this is best. Java, for instance, requires every non-public member to be declared with its own access specifier. Some other languages (I think D) do as well. An argument could be made that that's a far clearer style than using the specifier only when changing access. It seems to me that the most suitable approach will be different from one project to another, if not from one person to another. A diagnostic that recommends a particular style (or that helps with a specific kind of a project like the one you did for Qt) might be a good candidate for a plugin, but enshrining any one style (or a solution to a project-specific problem) in GCC as a general-purpose warning doesn't seem appropriate or in line with the definition of warnings in GCC: constructions that are not inherently erroneous but that are risky or suggest there may have been an error Martin PS There are other redundancies that some might say unnecessarily clutter code. For instance, declaring a symbol static in an unnamed namespace, or explicitly declaring a member function inline that's also defined within the body of a class, or explicitly declaring a function virtual that overrides one declared in a base class. None of these is diagnosed, and I'd say for good reason: they are all benign and do not suggest any sort of a coding mistake or present an opportunity for improvement. In fact, warning for some of them (especially the virtual function) might even be detrimental > Btw, the SonarAnalyzer also provides this warning to clean up code: > > https://sonarcloud.io/organizations/default/rules#rule_key=cpp%3AS3539 Thanks, that's an interesting data point. Martin > >> Btw., there are examples of poor coding practices where I can >> imagine a warning focused on access specifiers being helpful. >> For instance, a class whose member functions maintain non-trivial >> invariants among its data members should declare the data private >> to prevent clients from inadvertently breaking those invariants. >> A specific example might be a container class (like string or >> vector) that owns the block of memory it allocates. A warning >> that detected when such members are publicly accessible could >> help improve encapsulation. I suspect this would be challenging >> to implement and would likely require some non-trivial analysis >> in the middle end. > > That's way beyond the scope of what my warning is trying to achieve. > >> Another example is a class that defines an inaccessible member >> that isn't used (e.g., class A { int i; }; that Clang detects >> with -Wunused-private-field; or class A { void f () { } };). >> I would expect this to be doable in the front end. > > That's indeed a nice warning, too. > >> Martin > > Regards, > Volker >
On 23 Jul, Martin Sebor wrote: > On 07/23/2017 02:42 PM, Volker Reichelt wrote: >> On 21 Jul, Martin Sebor wrote: >>> On 07/20/2017 10:35 AM, 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; >>>> }; >>> >>> I'm very fond of warnings that help find real bugs, or even that >>> provide an opportunity to review code for potential problems or >>> inefficiencies and suggest a possibility to improve it in some >>> way (make it clearer, or easier for humans or compilers to find >>> real bugs in, or faster, etc.), even if the code isn't strictly >>> incorrect. >>> >>> In this case I'm having trouble coming up with an example where >>> the warning would have this effect. What do you have in mind? >>> (Duplicate access specifiers tend to crop up in large classes >>> and/or as a result of preprocessor conditionals.) >> >> This warning fights the "tend to crop up" effect that clutters the >> code. After some time these redundant access specifiers creep in >> and make your code harder to read. If I see something like >> >> ... >> void foo() const; >> >> private: >> void bar(); >> ... >> >> on the screen I tend to think that 'foo' has a different access >> level than bar. If that's not the case because the access-specifier >> is redundant, then that's just confusing and distracting. >> The warning helps to maintain readability of your code. >> >> The benefit might not be big, but the warning comes at relatively >> low cost. It passes a location around through the class stack and >> checks less than a handful of tokens. >> >> My personal motivation to implement this warning was the fact that >> I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT >> mechanism to the new function pointer syntax of Qt5. In the old >> version you had to mark slots in the following fashion: >> >> public slots: >> void foo(); >> void bar(); >> >> But now you can use any function making the 'slots' macro obsolete. >> Therefore I ended up with hundreds of redundant access-specifiers >> which this warning helped to clean up. Doing this sort of thing in the >> compiler with a proper parser is way safer than to write some script >> to achieve this. > > Okay, thanks for clarifying that. I think what's distracting to > one could be helpful to another. For example, it's not uncommon > for classes with many members to use redundant access specifiers > to group blocks of related declarations. Or, in a class with many > lines of comments (e.g., Doxygen), repeating the access specifier > every so often could be seen as helpful because otherwise there > would be no way to tell what its access is without scrolling up > or down. It's debatable what approach to dealing with this is > best. Java, for instance, requires every non-public member to > be declared with its own access specifier. Some other languages > (I think D) do as well. An argument could be made that that's > a far clearer style than using the specifier only when changing > access. It seems to me that the most suitable approach will be > different from one project to another, if not from one person to > another. A diagnostic that recommends a particular style (or that > helps with a specific kind of a project like the one you did for > Qt) might be a good candidate for a plugin, but enshrining any > one style (or a solution to a project-specific problem) in GCC > as a general-purpose warning doesn't seem appropriate or in line > with the definition of warnings in GCC: > > constructions that are not inherently erroneous but that are > risky or suggest there may have been an error > > Martin > > PS There are other redundancies that some might say unnecessarily > clutter code. For instance, declaring a symbol static in > an unnamed namespace, or explicitly declaring a member function > inline that's also defined within the body of a class, or > explicitly declaring a function virtual that overrides one > declared in a base class. None of these is diagnosed, and I'd > say for good reason: they are all benign and do not suggest any > sort of a coding mistake or present an opportunity for improvement. > In fact, warning for some of them (especially the virtual function) > might even be detrimental > >> Btw, the SonarAnalyzer also provides this warning to clean up code: >> >> https://sonarcloud.io/organizations/default/rules#rule_key=cpp%3AS3539 > > Thanks, that's an interesting data point. > > Martin Maybe the whole thing of diagnostics that refer more to style instead of correctness or potential bugs shoould be discussed (independent of my patch) by a broader audience. There are several tools out there like clang-tidy, SonarAnalyzer etc. that not only check for bugs but also flag stylistic issues. For GCC I could imagine a set of such wanings prefixed by -Wstyle- or something similar to make the intent clear. I'll post something on the main mailing list in the next days. Regards, Volker
Martin Sebor <msebor@gmail.com> writes: > On 07/23/2017 02:42 PM, Volker Reichelt wrote: >> On 21 Jul, Martin Sebor wrote: >>> On 07/20/2017 10:35 AM, 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; >>>> }; >>> >>> I'm very fond of warnings that help find real bugs, or even that >>> provide an opportunity to review code for potential problems or >>> inefficiencies and suggest a possibility to improve it in some >>> way (make it clearer, or easier for humans or compilers to find >>> real bugs in, or faster, etc.), even if the code isn't strictly >>> incorrect. >>> >>> In this case I'm having trouble coming up with an example where >>> the warning would have this effect. What do you have in mind? >>> (Duplicate access specifiers tend to crop up in large classes >>> and/or as a result of preprocessor conditionals.) >> >> This warning fights the "tend to crop up" effect that clutters the >> code. After some time these redundant access specifiers creep in >> and make your code harder to read. If I see something like >> >> ... >> void foo() const; >> >> private: >> void bar(); >> ... >> >> on the screen I tend to think that 'foo' has a different access >> level than bar. If that's not the case because the access-specifier >> is redundant, then that's just confusing and distracting. >> The warning helps to maintain readability of your code. >> >> The benefit might not be big, but the warning comes at relatively >> low cost. It passes a location around through the class stack and >> checks less than a handful of tokens. >> >> My personal motivation to implement this warning was the fact that >> I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT >> mechanism to the new function pointer syntax of Qt5. In the old >> version you had to mark slots in the following fashion: >> >> public slots: >> void foo(); >> void bar(); >> >> But now you can use any function making the 'slots' macro obsolete. >> Therefore I ended up with hundreds of redundant access-specifiers >> which this warning helped to clean up. Doing this sort of thing in the >> compiler with a proper parser is way safer than to write some script >> to achieve this. > > Okay, thanks for clarifying that. I think what's distracting to > one could be helpful to another. For example, it's not uncommon > for classes with many members to use redundant access specifiers > to group blocks of related declarations. Or, in a class with many > lines of comments (e.g., Doxygen), repeating the access specifier > every so often could be seen as helpful because otherwise there > would be no way to tell what its access is without scrolling up > or down. It's debatable what approach to dealing with this is > best. Java, for instance, requires every non-public member to > be declared with its own access specifier. Some other languages > (I think D) do as well. An argument could be made that that's > a far clearer style than using the specifier only when changing > access. It seems to me that the most suitable approach will be > different from one project to another, if not from one person to > another. A diagnostic that recommends a particular style (or that > helps with a specific kind of a project like the one you did for > Qt) might be a good candidate for a plugin, but enshrining any > one style (or a solution to a project-specific problem) in GCC > as a general-purpose warning doesn't seem appropriate or in line > with the definition of warnings in GCC: > > constructions that are not inherently erroneous but that are > risky or suggest there may have been an error I think there are some circumstances in which the warning would count, especially if you're working to a coding convention that requires all public members followed by all protected members followed by all private members. Having a duplicated specifier in that context might then indicate that you've got a cut-&-paste error. I think both that scenario and the ones Volker gave are enough justification for the warning to be useful, but not enough for including it in Wall or Wextra (which isn't being proposed). > PS There are other redundancies that some might say unnecessarily > clutter code. For instance, declaring a symbol static in > an unnamed namespace, or explicitly declaring a member function > inline that's also defined within the body of a class, or > explicitly declaring a function virtual that overrides one > declared in a base class. None of these is diagnosed, and I'd > say for good reason: they are all benign and do not suggest any > sort of a coding mistake or present an opportunity for improvement. > In fact, warning for some of them (especially the virtual function) > might even be detrimental Actually, I've wanted one for the "static in an anonymous namespace" in the past :-) But I think you could also say the same about things like -Wmissing-declarations. That warns about something as simple as: void foo(void) {} which I think is even harder to justify as being inherently suspicious. But in the ANSI C days, this was very useful in enforcing GCC's own coding convention, and so was enabled during bootstrap. Thanks, Richard
On 07/27/2017 01:13 PM, Richard Sandiford wrote: > Martin Sebor <msebor@gmail.com> writes: >> On 07/23/2017 02:42 PM, Volker Reichelt wrote: >>> On 21 Jul, Martin Sebor wrote: >>>> On 07/20/2017 10:35 AM, 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; >>>>> }; >>>> >>>> I'm very fond of warnings that help find real bugs, or even that >>>> provide an opportunity to review code for potential problems or >>>> inefficiencies and suggest a possibility to improve it in some >>>> way (make it clearer, or easier for humans or compilers to find >>>> real bugs in, or faster, etc.), even if the code isn't strictly >>>> incorrect. >>>> >>>> In this case I'm having trouble coming up with an example where >>>> the warning would have this effect. What do you have in mind? >>>> (Duplicate access specifiers tend to crop up in large classes >>>> and/or as a result of preprocessor conditionals.) >>> >>> This warning fights the "tend to crop up" effect that clutters the >>> code. After some time these redundant access specifiers creep in >>> and make your code harder to read. If I see something like >>> >>> ... >>> void foo() const; >>> >>> private: >>> void bar(); >>> ... >>> >>> on the screen I tend to think that 'foo' has a different access >>> level than bar. If that's not the case because the access-specifier >>> is redundant, then that's just confusing and distracting. >>> The warning helps to maintain readability of your code. >>> >>> The benefit might not be big, but the warning comes at relatively >>> low cost. It passes a location around through the class stack and >>> checks less than a handful of tokens. >>> >>> My personal motivation to implement this warning was the fact that >>> I changed a big Qt application suite from Qt's legacy SIGNAL-SLOT >>> mechanism to the new function pointer syntax of Qt5. In the old >>> version you had to mark slots in the following fashion: >>> >>> public slots: >>> void foo(); >>> void bar(); >>> >>> But now you can use any function making the 'slots' macro obsolete. >>> Therefore I ended up with hundreds of redundant access-specifiers >>> which this warning helped to clean up. Doing this sort of thing in the >>> compiler with a proper parser is way safer than to write some script >>> to achieve this. >> >> Okay, thanks for clarifying that. I think what's distracting to >> one could be helpful to another. For example, it's not uncommon >> for classes with many members to use redundant access specifiers >> to group blocks of related declarations. Or, in a class with many >> lines of comments (e.g., Doxygen), repeating the access specifier >> every so often could be seen as helpful because otherwise there >> would be no way to tell what its access is without scrolling up >> or down. It's debatable what approach to dealing with this is >> best. Java, for instance, requires every non-public member to >> be declared with its own access specifier. Some other languages >> (I think D) do as well. An argument could be made that that's >> a far clearer style than using the specifier only when changing >> access. It seems to me that the most suitable approach will be >> different from one project to another, if not from one person to >> another. A diagnostic that recommends a particular style (or that >> helps with a specific kind of a project like the one you did for >> Qt) might be a good candidate for a plugin, but enshrining any >> one style (or a solution to a project-specific problem) in GCC >> as a general-purpose warning doesn't seem appropriate or in line >> with the definition of warnings in GCC: >> >> constructions that are not inherently erroneous but that are >> risky or suggest there may have been an error > > I think there are some circumstances in which the warning would count, > especially if you're working to a coding convention that requires all > public members followed by all protected members followed by all private > members. Having a duplicated specifier in that context might then > indicate that you've got a cut-&-paste error. Perhaps. It sounds like a stretch to me. But my argument isn't that no styles exist that the warning might work for but rather that it tries to enforce a unique (and IMO, rare) style to the exclusion some common and arguably more useful ones. It's a wide spread convention to use an access specifier to introduce a block of related declarations. For example: class X { ... public: // public types ... public: // public functions ... private: // private functions ... private: // private data ... }; Although this is a reasonable convention I think it would be fair to consider a warning that diagnosed it if it had a good chance of finding real bugs in some other code. But this is not the case here (nor was the purpose of the warning to find such bugs). > I think both that scenario and the ones Volker gave are enough > justification for the warning to be useful, but not enough for > including it in Wall or Wextra (which isn't being proposed). > >> PS There are other redundancies that some might say unnecessarily >> clutter code. For instance, declaring a symbol static in >> an unnamed namespace, or explicitly declaring a member function >> inline that's also defined within the body of a class, or >> explicitly declaring a function virtual that overrides one >> declared in a base class. None of these is diagnosed, and I'd >> say for good reason: they are all benign and do not suggest any >> sort of a coding mistake or present an opportunity for improvement. >> In fact, warning for some of them (especially the virtual function) >> might even be detrimental > > Actually, I've wanted one for the "static in an anonymous namespace" > in the past :-) Out of curiosity, why? But again, I'm not arguing that there is no use case for finding these patterns. Just that they are benign and not suggestive of bugs or opportunities for improvements, and so not appropriate for inclusion in the form of warnings whose purpose is to point out likely coding mistakes that could be the source of bugs. > But I think you could also say the same about things like > -Wmissing-declarations. That warns about something as simple as: > > void foo(void) {} > > which I think is even harder to justify as being inherently suspicious. > But in the ANSI C days, this was very useful in enforcing GCC's own coding > convention, and so was enabled during bootstrap. Defining a function that's incompatible with its declaration in another translation unit is a common mistake in C (and undefined behavior). Using the function leads to hard to debug problems. Coding standard rules have been written (such as rule DCL40-C of the CERT Secure Coding Standard) and static analysis tools have been developed to detect this problem (see the Automated Detection section in the CERT rule). Many compilers implement this warning for this reason. Martin
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" } +};
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.