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

[Richard Sandiford]

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