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.
else if (TREE_CODE (t) == OVERLOAD)
+ t = OVL_FIRST (t) != conv_op_marker ? OVL_FIRST (t) : OVL_CHAIN (t);
Usually OVL_CHAIN will be another OVERLOAD, you want OVL_FIRST
(OVL_CHAIN (t)) in that case.
Jason
