On 23 Jul, Martin Sebor wrote:
> 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
>
> Martin
>
> 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
>
>> Btw, the SonarAnalyzer also provides this warning to clean up code:
>>
>> https://sonarcloud.io/organizations/default/rules#rule_key=cpp%3AS3539
>
> Thanks, that's an interesting data point.
>
> Martin
Maybe the whole thing of diagnostics that refer more to style instead of
correctness or potential bugs shoould be discussed (independent of my
patch) by a broader audience.
There are several tools out there like clang-tidy, SonarAnalyzer etc.
that not only check for bugs but also flag stylistic issues. For GCC I
could imagine a set of such wanings prefixed by -Wstyle- or something
similar to make the intent clear. I'll post something on the main
mailing list in the next days.
Regards,
Volker
