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.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.
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.reich...@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
