On 9/5/24 7:02 AM, Simon Martin wrote:
Hi Jason,
On 4 Sep 2024, at 18:09, Jason Merrill wrote:
On 9/1/24 2:51 PM, Simon Martin wrote:
Hi Jason,
On 26 Aug 2024, at 19:23, Jason Merrill wrote:
On 8/25/24 12:37 PM, Simon Martin wrote:
On 24 Aug 2024, at 23:59, Simon Martin wrote:
On 24 Aug 2024, at 15:13, Jason Merrill wrote:
On 8/23/24 12:44 PM, Simon Martin wrote:
We currently emit an incorrect -Woverloaded-virtual warning upon
the
following
test case
=== cut here ===
struct A {
virtual operator int() { return 42; }
virtual operator char() = 0;
};
struct B : public A {
operator char() { return 'A'; }
};
=== cut here ===
The problem is that warn_hidden relies on get_basefndecls to
find
the
methods
in A possibly hidden B's operator char(), and gets both the
conversion operator
to int and to char. It eventually wrongly concludes that the
conversion to int
is hidden.
This patch fixes this by filtering out conversion operators to
different types
from the list returned by get_basefndecls.
Hmm, same_signature_p already tries to handle comparing
conversion
operators, why isn't that working?
It does indeed.
However, `ovl_range (fns)` does not only contain `char
B::operator()` -
for which `any_override` gets true - but also `conv_op_marker` -
for
which `any_override` gets false, causing `seen_non_override` to
get
to
true. Because of that, we run the last loop, that will emit a
warning
for all `base_fndecls` (except `char B::operator()` that has been
removed).
We could test `fndecl` and `base_fndecls[k]` against
`conv_op_marker` in
the loop, but we’d still need to inspect the “converting to”
type
in the last loop (for when `warn_overloaded_virtual` is 2). This
would
make the code much more complex than the current patch.
Makes sense.
It would however probably be better if `get_basefndecls` only
returned
the right conversion operator, not all of them. I’ll draft
another
version of the patch that does that and submit it in this thread.
I have explored my suggestion further and it actually ends up more
complicated than the initial patch.
Yeah, you'd need to do lookup again for each member of fns.
Please find attached a new revision to fix the reported issue, as
well
as new ones I discovered while testing with -Woverloaded-virtual=2.
It’s pretty close to the initial patch, but (1) adds a missing
“continue;” (2) fixes a location problem when
-Woverloaded-virtual==2 (3) adds more test cases. The commit log is
also
more comprehensive, and should describe well the various problems
and
why the patch is correct.
+ if (IDENTIFIER_CONV_OP_P (name)
+ && !same_type_p (DECL_CONV_FN_TYPE (fndecl),
+ DECL_CONV_FN_TYPE (base_fndecls[k])))
+ {
+ base_fndecls[k] = NULL_TREE;
+ continue;
+ }
So this removes base_fndecls[k] if it doesn't return the same type
as
fndecl. But what if there's another conversion op in fns that does
return the same type as base_fndecls[k]?
If I add an operator int() to both base and derived in
Woverloaded-virt7.C, the warning disappears.
That was an issue indeed. I’ve reworked the patch, and came up with
the attached latest version. It explicitly keeps track both of
overloaded and of hidden base methods (and the “hiding method”
for
the latter), and uses those instead of juggling with bools and
nullified
base_decls.
On top of fixing the issue the PR reports, it fixes a few that I came
across while investigating:
- wrongly emitting the warning if the base method is not virtual (the
lines added to Woverloaded-virt1.C would cause a warning without the
patch)
- wrongly emitting the warning when the derived class method is a
template, which is wrong since template members don’t override
virtual
base methods (see the change in pr61945.C)
This change seems wrong to me; the warning is documented as "Warn when
a function declaration hides virtual functions from a base class," and
templates can certainly hide virtual base methods, as indeed they do
in that testcase.
Gasp, you’re right. The updated patch fixes this by simply working
from the TEMPLATE_TEMPLATE_RESULT of TEMPLATE_DECL; so pr61945.C warns
again (after changing the signature so that it actually hides the base
class; it was not before, hence the warning was actually incorrect).
It was hiding the base function before, the warning was correct; hiding
is based on name, not signature. Only overriding depends on the signature.
Jason
