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