[PATCH] New C++ warning option '-Wduplicated-access-specifiers'

[Volker Reichelt]

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