On 10/7/24 11:27 AM, Simon Martin wrote:
Hi Jason,
On 17 Sep 2024, at 18:41, Jason Merrill wrote:
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.
Indeed, good call, it makes things much easier. The attached patch does
this, and has been successfully tested on x86_64-pc-linux-gnu. Ok for
trunk?
+ else if (IDENTIFIER_CONV_OP_P (name)
+ && !same_type_p (DECL_CONV_FN_TYPE (fndecl),
+ DECL_CONV_FN_TYPE
(base_fndecls[k])))
You decided not to compare DECL_NAME?
/* Now give a warning for all base functions without overriders,
as they are hidden. */
for (tree base_fndecl : base_fndecls)
+ {
+ if (!base_fndecl || overriden_base_fndecls.contains (base_fndecl))
+ continue;
+ tree *hider = hidden_base_fndecls.get (base_fndecl);
+ if (hider)
How about looping over hidden_base_fndecls instead of base_fndecls?
Jason
