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.
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'; }
>> +};
