Hi Jason,
On 24 Aug 2024, at 23:59, Simon Martin wrote:
> Hi Jason,
>
> 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.
>
> 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.
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.
Successfully tested on x86_64-pc-linux-gnu; OK for trunk?
Thanks!
Simon
>
>>> Successfully tested on x86_64-pc-linux-gnu.
>>>
>>> PR c++/109918
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> * class.cc (warn_hidden): Filter out conversion operators to
>>> different
>>> types.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * g++.dg/warn/Woverloaded-virt5.C: New test.
>>>
>>> ---
>>> gcc/cp/class.cc | 33
>>> ++++++++++++-------
>>> gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C | 12 +++++++
>>> 2 files changed, 34 insertions(+), 11 deletions(-)
>>> create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
>>>
>>> diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
>>> index fb6c3370950..a8178a31fe8 100644
>>> --- a/gcc/cp/class.cc
>>> +++ b/gcc/cp/class.cc
>>> @@ -3267,18 +3267,29 @@ warn_hidden (tree t)
>>> if (TREE_CODE (fndecl) == FUNCTION_DECL
>>> && DECL_VINDEX (fndecl))
>>> {
>>> - /* 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])
>>> + continue;
>>> + /* If FNS is a conversion operator, base_fndecls contains
>>> + all conversion operators from base classes; we need to
>>> + remove those converting to a different type. */
>>> + 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;
>>> + }
>>> + /* If the method from the base class has the same signature
>>> + as the method from the derived class, it has been
>>> + overriden. Note that we can't move on after finding
>>> + one match: fndecl might override multiple base fns. */
>>> + else if (same_signature_p (fndecl, base_fndecls[k]))
>>> + {
>>> + base_fndecls[k] = NULL_TREE;
>>> + any_override = true;
>>> + }
>>> + }
>>> }
>>> if (!any_override)
>>> seen_non_override = true;
>>> 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..ea26569e565
>>> --- /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 }
>>> +
>>> +struct A {
>>> + virtual operator int() { return 42; }
>>> + virtual operator char() = 0;
>>> +};
>>> +
>>> +struct B : public A {
>>> + operator char() { return 'A'; }
>>> +};
From 5efe3910b4cd98a6565cdea4a977f9349a1d871f 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 first 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. The
patch fixes both this and the first problem by nullifying conversion
operators to different types in base_fndecls, and validates both fixes
via Woverloaded-virt5.C and Woverloaded-virt6.C.
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. The patch fixes this by going over
conv_op_marker in location_of, and validates the fix via
Woverloaded-virt7.C.
Successfully tested on x86_64-pc-linux-gnu.
PR c++/109918
gcc/cp/ChangeLog:
* class.cc (warn_hidden): Ignore conversion operators to different
types.
* error.cc (location_of): Skip over conv_op_marker.
gcc/testsuite/ChangeLog:
* g++.dg/warn/Woverloaded-virt5.C: New test.
* g++.dg/warn/Woverloaded-virt6.C: New test.
* g++.dg/warn/Woverloaded-virt7.C: New test.
---
gcc/cp/class.cc | 28 ++++++++++++++-----
gcc/cp/error.cc | 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 | 12 ++++++++
5 files changed, 58 insertions(+), 8 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
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index fb6c3370950..bf7dfc5a8cf 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -3271,14 +3271,28 @@ warn_hidden (tree t)
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. */
+ multiple base fns.
+ If the signatures are different and the method is a
+ conversion operator, the base and derived methods
+ might convert to a different type. In this case, the
+ base one should be ignored. */
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])
+ continue;
+ 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;
+ }
+ else if (same_signature_p (fndecl, base_fndecls[k]))
+ {
+ base_fndecls[k] = NULL_TREE;
+ any_override = true;
+ }
+ }
}
if (!any_override)
seen_non_override = true;
diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc
index 879e5a115cf..c0c8df31db8 100644
--- a/gcc/cp/error.cc
+++ b/gcc/cp/error.cc
@@ -3366,7 +3366,7 @@ 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_CHAIN (t);
if (DECL_P (t))
return DECL_SOURCE_LOCATION (t);
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..8f47e080f8c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
@@ -0,0 +1,12 @@
+// 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" }
+};
--
2.44.0
