This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] New C++ warning option '-Wduplicated-access-specifiers'
On 07/27/2017 01:13 PM, Richard Sandiford wrote:
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.
Perhaps. It sounds like a stretch to me. But my argument isn't
that no styles exist that the warning might work for but rather
that it tries to enforce a unique (and IMO, rare) style to the
exclusion some common and arguably more useful ones. It's a wide
spread convention to use an access specifier to introduce a block
of related declarations. For example:
class X {
...
public: // public types
...
public: // public functions
...
private: // private functions
...
private: // private data
...
};
Although this is a reasonable convention I think it would be fair
to consider a warning that diagnosed it if it had a good chance
of finding real bugs in some other code. But this is not the case
here (nor was the purpose of the warning to find such bugs).
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 :-)
Out of curiosity, why?
But again, I'm not arguing that there is no use case for finding
these patterns. Just that they are benign and not suggestive of
bugs or opportunities for improvements, and so not appropriate
for inclusion in the form of warnings whose purpose is to point
out likely coding mistakes that could be the source of bugs.
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.
Defining a function that's incompatible with its declaration in
another translation unit is a common mistake in C (and undefined
behavior). Using the function leads to hard to debug problems.
Coding standard rules have been written (such as rule DCL40-C of
the CERT Secure Coding Standard) and static analysis tools have
been developed to detect this problem (see the Automated Detection
section in the CERT rule). Many compilers implement this warning
for this reason.
Martin