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).
Successfully tested on x86_64-pc-linux-gnu; OK for trunk?
Thanks, Simon
>
>> - 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 40ef56e5d1f56b2176d87e7fec5a2a51780351b2 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: Change signature to really hide base
method.
* 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.
---
gcc/cp/class.cc | 70 ++++++++++++-------
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 | 25 +++++++
gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C | 15 ++++
gcc/testsuite/g++.dg/warn/pr61945.C | 2 +-
8 files changed, 115 insertions(+), 26 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
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index fb6c3370950..850a2f14a03 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,64 @@ 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) == TEMPLATE_DECL)
+ fndecl = DECL_TEMPLATE_RESULT (fndecl);
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 4a9e9aa3cdc..401dbb43e56 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..01cd562609a
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C
@@ -0,0 +1,12 @@
+// PR c++/109918 - Exact PR testcase
+// { dg-do compile }
+// { dg-additional-options -Wall }
+
+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..c18049e3a92
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C
@@ -0,0 +1,12 @@
+// PR c++/109918 - PR testcase with -Woverloaded-virtual=2
+// { 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..be50c890d20
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C
@@ -0,0 +1,25 @@
+// PR c++/109918 - Test different CV-quals
+// { 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" }
+ 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-virt8.C
b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C
new file mode 100644
index 00000000000..51af2dae77c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.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..f7b1f5fc442 100644
--- a/gcc/testsuite/g++.dg/warn/pr61945.C
+++ b/gcc/testsuite/g++.dg/warn/pr61945.C
@@ -7,5 +7,5 @@ class A {
};
class B : A {
template <typename>
- void foo (); // { dg-message "by .*B::foo\\(\\)." }
+ void foo (int); // { dg-message "by .*B::foo\\(int\\)." }
};
--
2.44.0
