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
