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)
- an invalid early return - see Woverloaded-virt9.C,
- reporting the first overload instead of the one that actually hides -
see the dg-note in Woverloaded-virt8.C that’d fail without the patch
because we’d report the int overload)
Successfully tested on x86_64-pc-linux-gnu; OK for trunk?
>> 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.
Thanks. Even though not strictly needed anymore by the updated patch,
I’m still including the (fixed) change in the patch.
Simon
From 2dd66681842725649abdaef0eec3df553264fb80 Mon Sep 17 00:00:00 2001
From: Simon Martin <si...@nasilyan.com>
Date: Sun, 25 Aug 2024 15:17:47 +0200
Subject: [PATCH] c++: Fix overeager Woverloaded-virtual with conversion
operators [PR109918]
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 when iterating over ovl_range (fns), warn_hidden
gets confused by the conversion operator marker, concludes that
seen_non_override is true and therefore emits a warning for all
conversion operators in A that do not convert to char, even if
-Woverloaded-virtual is 1 (e.g. with -Wall, the case reported).
A second set of problems is highlighted when -Woverloaded-virtual is 2.
First, with the same test case, since base_fndecls contains all
conversion operators in A (except the one to char, that's been removed
when iterating over ovl_range (fns)), we emit a spurious warning for
the conversion operator to int, even though it's not overriden.
Second, in case there are several conversion operators with different
cv-qualifiers to the same type in A, we rightfully emit a warning,
however the note uses the location of the conversion operator marker
instead of the right one; location_of should go over conv_op_marker.
This patch fixes all these by explicitly keeping track of (1) base
methods that are overriden, as well as (2) base methods that are hidden
but not overriden (and by what), and warning about methods that are in
(2) but not (1). It also ignores non virtual base methods, per
"definition" of -Woverloaded-virtual.
Successfully tested on x86_64-pc-linux-gnu.
PR c++/109918
gcc/cp/ChangeLog:
* class.cc (warn_hidden): Keep track of overloaded and of hidden
base methods. Mention the actual hiding function in the warning,
not the first overload.
* error.cc (location_of): Skip over conv_op_marker.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Woverloaded-virt1.C: Check that no warning is
emitted for non virtual base methods.
* g++.dg/warn/pr61945.C: Adjust test expectations, since
template methods don't override base methods.
* g++.dg/warn/Woverloaded-virt5.C: New test.
* g++.dg/warn/Woverloaded-virt6.C: New test.
* g++.dg/warn/Woverloaded-virt7.C: New test.
* g++.dg/warn/Woverloaded-virt8.C: New test.
* g++.dg/warn/Woverloaded-virt9.C: New test.
---
gcc/cp/class.cc | 68 ++++++++++++-------
gcc/cp/error.cc | 3 +-
gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C | 2 +
gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C | 12 ++++
gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C | 12 ++++
gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C | 21 ++++++
gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C | 25 +++++++
gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C | 15 ++++
gcc/testsuite/g++.dg/warn/pr61945.C | 6 +-
9 files changed, 137 insertions(+), 27 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C
create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C
create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index fb6c3370950..da214b7af53 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3238,10 +3238,15 @@ warn_hidden (tree t)
continue;
tree name = OVL_NAME (fns);
+ size_t num_fns = 0; /* The number of fndecls in fns. */
auto_vec<tree, 20> base_fndecls;
tree base_binfo;
tree binfo;
unsigned j;
+ hash_set<tree> overriden_base_fndecls;
+ /* base_fndecls that are hidden but not overriden. The "value"
+ contains the fndecl that hides the "key". */
+ hash_map<tree, tree> hidden_base_fndecls;
if (IDENTIFIER_CDTOR_P (name))
continue;
@@ -3259,47 +3264,62 @@ warn_hidden (tree t)
if (base_fndecls.is_empty ())
continue;
- /* Remove any overridden functions. */
- bool seen_non_override = false;
+ /* Find all the base_fndecls that are overridden, as well as those
+ that are hidden, in T. */
for (tree fndecl : ovl_range (fns))
{
- bool any_override = false;
if (TREE_CODE (fndecl) == FUNCTION_DECL
- && DECL_VINDEX (fndecl))
+ && fndecl != conv_op_marker)
{
+ ++num_fns;
/* If the method from the base class has the same
signature as the method from the derived class, it
has been overridden. Note that we can't move on
after finding one match: fndecl might override
multiple base fns. */
for (size_t k = 0; k < base_fndecls.length (); k++)
- if (base_fndecls[k]
- && same_signature_p (fndecl, base_fndecls[k]))
- {
- base_fndecls[k] = NULL_TREE;
- any_override = true;
- }
+ {
+ if (!base_fndecls[k] || !DECL_VINDEX (base_fndecls[k]))
+ continue;
+ else if (same_signature_p (fndecl, base_fndecls[k]))
+ overriden_base_fndecls.add (base_fndecls[k]);
+ else if (IDENTIFIER_CONV_OP_P (name))
+ {
+ if (same_type_p (DECL_CONV_FN_TYPE (fndecl),
+ DECL_CONV_FN_TYPE (base_fndecls[k])))
+ /* fndecl and base_fndecls[k] convert to the same
+ type, but have different CV qualifiers, so fndecl
+ hides base_fndecl[k]. */
+ hidden_base_fndecls.put (base_fndecls[k], fndecl);
+ }
+ else
+ hidden_base_fndecls.put (base_fndecls[k], fndecl);
+ }
}
- if (!any_override)
- seen_non_override = true;
}
- if (!seen_non_override && warn_overloaded_virtual == 1)
- /* All the derived fns override base virtuals. */
- return;
+ if (warn_overloaded_virtual == 1
+ && overriden_base_fndecls.elements () == num_fns)
+ /* All the fns override a base virtual. */
+ continue;
/* Now give a warning for all base functions without overriders,
as they are hidden. */
for (tree base_fndecl : base_fndecls)
- if (base_fndecl)
- {
- auto_diagnostic_group d;
- /* Here we know it is a hider, and no overrider exists. */
- if (warning_at (location_of (base_fndecl),
- OPT_Woverloaded_virtual_,
- "%qD was hidden", base_fndecl))
- inform (location_of (fns), " by %qD", fns);
- }
+ {
+ if (!base_fndecl || overriden_base_fndecls.contains (base_fndecl))
+ continue;
+ tree *hider = hidden_base_fndecls.get (base_fndecl);
+ if (hider)
+ {
+ auto_diagnostic_group d;
+ /* Here we know it is a hider, and no overrider exists. */
+ if (warning_at (location_of (base_fndecl),
+ OPT_Woverloaded_virtual_,
+ "%qD was hidden", base_fndecl))
+ inform (location_of (*hider), " by %qD", *hider);
+ }
+ }
}
}
diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
index 420fad26b7b..eb059c3b451 100644
--- a/gcc/cp/error.cc
+++ b/gcc/cp/error.cc
@@ -3396,7 +3396,8 @@ location_of (tree t)
return input_location;
}
else if (TREE_CODE (t) == OVERLOAD)
- t = OVL_FIRST (t);
+ t = OVL_FIRST (t) != conv_op_marker ? OVL_FIRST (t)
+ : OVL_FIRST (OVL_CHAIN (t));
if (DECL_P (t))
return DECL_SOURCE_LOCATION (t);
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C
b/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C
index 92f8327b9d0..9091bfabc96 100644
--- a/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C
@@ -5,10 +5,12 @@ class Foo
{
public:
virtual void f(int); // { dg-warning "hidden" }
+ void g(int); // Not virtual, so no warning
};
class Bar : public Foo
{
public:
virtual void f(short); // { dg-message "by" }
+ virtual void g(short);
};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
new file mode 100644
index 00000000000..0c954b1930f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
@@ -0,0 +1,12 @@
+// PR c++/109918
+// { dg-do compile }
+// { dg-additional-options -Woverloaded-virtual=1 }
+
+struct A {
+ virtual operator int() { return 42; }
+ virtual operator char() = 0;
+};
+
+struct B : public A {
+ operator char() { return 'A'; }
+};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C
b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C
new file mode 100644
index 00000000000..b99b9933688
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C
@@ -0,0 +1,12 @@
+// PR c++/109918
+// { dg-do compile }
+// { dg-additional-options -Woverloaded-virtual=2 }
+
+struct A {
+ virtual operator int() { return 42; }
+ virtual operator char() = 0;
+};
+
+struct B : public A {
+ operator char() { return 'A'; }
+};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
new file mode 100644
index 00000000000..ece948fff72
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
@@ -0,0 +1,21 @@
+// PR c++/109918
+// { dg-do compile }
+// { dg-additional-options -Woverloaded-virtual }
+
+struct A {
+ virtual operator char() { return 'a'; }
+ virtual operator char() const { return 'b'; } // { dg-warning "was hidden" }
+};
+
+struct B : public A {
+ operator char() { return 'A'; } // { dg-note "by" }
+};
+
+struct AA {
+ virtual char func(char) { return 'a'; }
+ virtual char func(char) const { return 'b'; } // { dg-warning "was hidden" }
+};
+
+struct BB : public AA {
+ char func(char) { return 'A'; } // { dg-note "by" }
+};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C
b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C
new file mode 100644
index 00000000000..5fec6ee5c57
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C
@@ -0,0 +1,25 @@
+// PR c++/109918
+// { dg-do compile }
+// { dg-additional-options -Woverloaded-virtual=2 }
+
+struct A {
+ virtual operator char() { return 'a'; }
+ virtual operator char() const { return 'b'; } // { dg-warning "was hidden" }
+ virtual operator int() { return 42; }
+};
+
+struct B : public A {
+ operator char() { return 'A'; } // { dg-note "by" }
+ operator int() { return 43; }
+};
+
+struct AA {
+ virtual char func (char) { return 'a'; }
+ virtual char func(char) const { return 'b'; } // { dg-warning "was hidden" }
+ virtual int func(int) { return 42; }
+};
+
+struct BB : public AA {
+ char func(char) { return 'A'; } // { dg-note "by" }
+ int func(int) { return 43; }
+};
diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C
b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C
new file mode 100644
index 00000000000..51af2dae77c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C
@@ -0,0 +1,15 @@
+// Identified when investigating PR c++/109918: no warning was emitted due to
+// an incorrect early return in warn_hidden.
+// { dg-additional-options -Wall }
+
+struct Foo
+{
+ virtual void f(int); // { dg-warning "hidden" }
+ virtual void g() {}
+};
+
+struct Bar : Foo
+{
+ virtual void f(short); // { dg-message "by" }
+ virtual void g() {}
+};
diff --git a/gcc/testsuite/g++.dg/warn/pr61945.C
b/gcc/testsuite/g++.dg/warn/pr61945.C
index 2252330835f..2dc34bcc475 100644
--- a/gcc/testsuite/g++.dg/warn/pr61945.C
+++ b/gcc/testsuite/g++.dg/warn/pr61945.C
@@ -2,10 +2,12 @@
// { dg-do compile }
// { dg-options "-Woverloaded-virtual" }
+// No warning because template members don't override virtual base methods
+
class A {
- virtual int foo (); // { dg-warning "was hidden" }
+ virtual int foo ();
};
class B : A {
template <typename>
- void foo (); // { dg-message "by .*B::foo\\(\\)." }
+ int foo (int);
};
--
2.44.0
