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