On 9/17/24 10:38 AM, Simon Martin wrote:
Hi Jason,
Apologies for the back and forth and thanks for your patience!
No worries.
On 5 Sep 2024, at 19:00, Jason Merrill wrote:
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.
Indeed. The mistake in the last patch was to assume that
same_signature_p was considering return types... I have fixed this wrong
assumption in the attached updated revision, successfully tested on
x86_64-pc-linux-gnu; OK for trunk?
Hmm, I don't see how that assumption was problematic?
+ if (!base_fndecls[k] || !DECL_VINDEX (base_fndecls[k]))
+ continue;
+ bool same_signature = same_signature_p (fndecl,
+ base_fndecls[k]);
+ bool same_return_type = IDENTIFIER_CONV_OP_P (name)
+ ? same_type_p (DECL_CONV_FN_TYPE (fndecl),
+ DECL_CONV_FN_TYPE (base_fndecls[k]))
+ : same_type_p (TREE_TYPE (TREE_TYPE (fndecl)),
+ TREE_TYPE (TREE_TYPE (base_fndecls[k])));
+ if (IDENTIFIER_CONV_OP_P (name) && !same_return_type)
+ ; /* Conversion operators to different types are
+ unrelated. */
+ else if (same_signature && same_return_type)
They don't need to have the same return type to override, just the same
signature; see covariant return types. The return type only matters for
conversion functions, because that's what determines hiding.
And since the return type of a conversion function is encoded in its
DECL_NAME, we should be able to simplify the first case to
if (DECL_NAME (base_fndecls[k]) != DECL_NAME (fndecl))
Also, it might be easier to check for override by comparing DECL_VINDEX.
+ overriden_base_fndecls.add (base_fndecls[k]);
+ else
+ hidden_base_fndecls.put (base_fndecls[k], fndecl);
