[PATCH v6] c++: Fix overeager Woverloaded-virtual with conversion operators [PR109918]
Simon Martin
simon@nasilyan.com
Mon Oct 7 15:27:00 GMT 2024
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?
Thanks, Simon
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 0001-c-Fix-overeager-Woverloaded-virtual-with-conversion-.patch
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20241007/2876643a/attachment.ksh>
More information about the Gcc-patches
mailing list