https://gcc.gnu.org/bugzilla/show_bug.cgi?id=121640
Bug ID: 121640
Summary: -Woverloaded-virtual is probably bad
Product: gcc
Version: unknown
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: c++
Assignee: unassigned at gcc dot gnu.org
Reporter: quirkygnu at bladeshadow dot org
Target Milestone: ---
I think the warning, overloaded virtual, may be a case of the cure being worse
than the disease. I believe the warning is largely superfluous (which I'll
expand on momentarily), but much more importantly, the warning, or at least its
proposed solution in the compiler documentation, can actually be dangerous.
That's a much more serious problem so I'll deal with that first.
One of the main points of inheritance is to reduce code and increase reuse, by
inheriting what is common, and then overriding (not necessarily overloading)
what is not. Consider the following (admittedly contrived and oversimple)
example code:
$ sed 's/^/ /' override.cc
#include <iostream>
class Request {
protected:
int option;
public:
Request() {}
virtual void set_options(int foo) { option = foo; }
virtual void execute() { std::cout << "option is " << option <<
std::endl; }
};
class SpecialRequest : public Request {
protected:
char *thing;
public:
// Use the GCC-documented solution
using Request::set_options;
virtual void set_options(int foo, char *name) { option = foo; }
virtual void execute() { std::cout << "option for " << thing << " is "
<< option << std::endl; }
};
int main(int argc, char **argv)
{
char *buff = NULL;
SpecialRequest R;
// This is a bug
R.set_options(1);
R.execute();
}
If SpecialRequest does not include the line "using Request::set_options;" then
this code generates the warning. However it clearly is not meant to include
the base class' method, and doing so, in this instance, creates the opportunity
for undefined behavior, a crash, or worst of all, a potential security hole.
The above will compile with no warnings, however when R.execute() is called,
R.thing is uninitialized, and the code will crash or otherwise behave
unreliably. The code is also conceptually broken: Clearly it was never the
intent of the author to allow the base class' method to be called in the
derived class.
In environments such as mine, where by policy we must always compile with -Wall
-Werror, this can lead to less experienced engineers blindly following the
advice to import the base class method into the derived class, leading to
potential problems.
Hiding a base class' implementation is often desirable, and even required, in
order to ensure your design is correct and does not create situations like the
above. It's an essential feature of inheritance.
Now, I also said I think the warning is likely superfluous. There seem to be
basically 4 cases:
1. If you call SpecialRequest::set_options() with only an int argument, you
will
get a compiler error. No warning is required.
2. If you call Request::set_options() with a char* argument, you will get a
compiler error. No warning is required.
3. If SpecialRequest::set_options() declares parameters that the base class'
method does not have, but uses them, then clearly they are intentional and
they are required arguments. No warning is required.
4. Finally, SpecialRequest::set_options() unintentionally declared parameters
that the base class doesn't, and did not use them. It appears the warning
was created predominantly to catch this case. However, for this case we
already have -Wunused-parameter so once again, this warning is not required.
This leads me to conclude that -Woverloaded-virtual is unnecessary, and trying
to resolve it in the officially-documented way could lead to dangerous code, so
it should be removed. Or, at the very least, -Wall should not include it.
