Friendly ping. Thanks! Simon
On 16 Oct 2024, at 17:43, Simon Martin wrote:
> Hi Jason,
>
> On 12 Oct 2024, at 4:51, Jason Merrill wrote:
>
>> On 10/11/24 7:02 AM, Simon Martin wrote:
>>> Hi Jason,
>>>
>>> On 11 Oct 2024, at 0:35, Jason Merrill wrote:
>>>
>>>> On 10/7/24 3:35 PM, Simon Martin wrote:
>>>>> On 7 Oct 2024, at 18:58, Jason Merrill wrote:
>>>>>> On 10/7/24 11:27 AM, Simon Martin wrote:
>>>>
>>>>>>> /* Now give a warning for all base functions without overriders,
>>>>>>> as they are hidden. */
>>>>>>> for (tree base_fndecl : base_fndecls)
>>>>>>> + {
>>>>>>> + if (!base_fndecl || overriden_base_fndecls.contains
>>>>>>> (base_fndecl))
>>>>>>> + continue;
>>>>>>> + tree *hider = hidden_base_fndecls.get (base_fndecl);
>>>>>>> + if (hider)
>>>>>>
>>>>>> How about looping over hidden_base_fndecls instead of base_fndecls?
>>>>
>>>>> Unfortunately it does not work because a given base method can be
>>>>> hidden
>>>>> by one overload and overriden by another, in which case we don’t
>>>>> want
>>>>> to warn (see for example AA:foo(int) in Woverloaded-virt7.C). So we
>
>>>>> need
>>>>> to take both collections into account.
>>>>
>>>> Yes, you'd still need to check overridden_base_fndecls.contains, but
>
>>>> that doesn't seem any different iterating over hidden_base_fndecls
>>>> instead of base_fndecls.
>>> Sure, and I guess iterating over hidden_base_fndecls is more coherent
>
>>>
>>> with what the warning is about. Changed in the attached updated patch,
>>> successfully tested on x86_64-pc-linux-gnu. OK?
>>
>> OK, thanks.
> As you know the patch had to be reverted due to PR117114, that
> highlighted a bunch of issues with comparing DECL_VINDEXes: it might
> give false positives in case of multiple inheritance (the case in
> PR117114), but also if there’s single inheritance by the hierarchy has
> more than two levels (another issue I found while bootstrapping with
> rust enabled).
>
> The attached updated patch introduces an overrides_p function, based on
> the existing check_final_overrider, and uses it when the signatures match.
>
> It’s been successfully tested on x86_64-pc-linux-gnu, and bootstrap
> works fine with —enable-languages=all (and rust properly configured, so
> included here). OK for trunk?
>
> Thanks, Simon
