Hi Jason,
On 4 Feb 2025, at 1:10, Jason Merrill wrote:
> On 1/31/25 12:11 PM, Simon Martin wrote:
>> Hi Jason,
>>
>> On 27 Jan 2025, at 16:49, Jason Merrill wrote:
>>
>>> On 1/27/25 10:41 AM, Simon Martin wrote:
>>>> Hi Jason,
>>>>
>>>> On 17 Jan 2025, at 23:33, Jason Merrill wrote:
>>>>
>>>>> On 1/17/25 9:52 AM, Simon Martin wrote:
>>>>>> Hi Jason,
>>>>>>
>>>>>> On 16 Jan 2025, at 22:49, Jason Merrill wrote:
>>>>>>
>>>>>>> On 10/16/24 11:43 AM, Simon Martin wrote:
>>>>>>>> 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).
>>>>>>>
>>>>>>> Yes, relying on DECL_VINDEX equality was wrong, sorry to mislead
>>>>>>> you.
>>>>>>>
>>>>>>>> The attached updated patch introduces an overrides_p function,
>>>>>>>> based
>>>>>>>> on
>>>>>>>> the existing check_final_overrider, and uses it when the
>>>>>>>> signatures
>>>>
>>>>>>>> match.
>>>>>>>
>>>>>>> That seems unnecessary. It seems like removing that only breaks
>>>>>>> Woverloaded-virt11.C, and making that work again only requires
>>>>>>> bringing back the check that DECL_VINDEX (fndecl) is set (to any
>>>>>>> value). Or remembering that fndecl was a template, so it can't
>>>>>>> really
>>>>>>> have the same signature as a non-template, whatever
>>>>>>> same_signature_p
>>>>
>>>>>>> says.
>>>>>> That’s right, only Woverloaded-virt11.C fails without the
>>>>>> check_final_overrider call.
>>>>>>
>>>>>> Thanks for the suggestion to check whether fndecl is a template.
>>
>>>>>> This
>>>>>> is
>>>>>> what the updated attached patch does, successfully tested on
>>>>>> x86_64-pc-linux-gnu.
>>>>>>
>>>>>> OK for GCC 15? And if so, thoughts on backporting to release
>>>>>> branches
>>>>>> (technically it’s a regression but it’s “just” an
>>>>>> incorrect
>>>>>> warning fix, so probably not worth the risk)?
>>>>>
>>>>> Right, I wouldn't backport.
>>>>>
>>>>>> + if (warn_overloaded_virtual == 1
>>>>>> + && overrider_fndecls.elements () == num_fns)
>>>>>> + /* All the fns override a base virtual. */
>>>>>> + continue;
>>>>>
>>>>> This looks like the only use of the overrider_fndecls hash_set. A
>>>>> hash_set seems a bit overkill for checking whether everything in
>>>>> fns
>>
>>>>> is an overrider; keeping track of how many times the old
>>>>> any_override
>>>>> was set should work just as well?
>>>> Yeah you’re right :-/ I’ve changed my latest patch to simply
>>>> count
>>>> overriders.
>>>>
>>>>>> + /* fndecls hides base_fndecls[k]. */
>>>>>> + auto_vec<tree> &hiders =
>>>>>> + hidden_base_fndecls.get_or_insert
>>>>>> (base_fndecls[k]);
>>>>>> + if (!hiders.contains (fndecl))
>>>>>> + hiders.safe_push (fndecl);
>>>>>
>>>>> Hmm, do you think users want a full list of the overloads that
>>>>> don't
>>
>>>>> override? I'd think the problem is more the overload that doesn't
>>
>>>>> exist rather than the ones that do. The current code ends up in
>>>>> the
>>
>>>>> OVERLOAD handling of dump_decl that just prints scope::name.
>>>> Indeed, the full list is probably not super useful... One problem
>>>> with
>>>> the current code is that for conversion operators, it will give a
>>>> note
>>>> such as “note: by 'operator’”, so I propose to keep track
>>>> of
>>>> at
>>>> least one of the hiders, and use it to show the note (and get a
>>>> proper
>>>> “by 'virtual B::operator char()'” note for conversion
>>>> operators).
>>>>
>>>> Hence the updated patch, successfully tested on
>>>> x86_64-pc-linux-gnu.
>>>> Ok
>>>> for trunk?
>>>
>>>> + else if (!template_p /* Template methods don't override. */
>>>> + && same_signature_p (fndecl, base_fndecls[k]))
>>>> + {
>>>> + overriden_base_fndecls.add (base_fndecls[k]);
>>>> + ++num_overriders;
>>>> + }
>>>
>>> I'm concerned that this will increment num_overriders multiple times
>>> for a single fndecl if it overrides functions in multiple bases.
>> Such a case is covered by the new Woverloaded-virt11.C and does not
>> warn, but it’s true that we don’t take the “if
>> (warn_overloaded_virtual == 1 && num_overriders == num_fns)”
>> continue,
>> and we should - thanks.
>>
>> I have updated the patch to only increment num_overriders at the end
>> of
>> the loop iterating on base functions if we’ve seen at least one
>> overridden base function. Successfully tested on x86_64-pc-linux-gnu.
>> OK
>> for trunk?
>
>> @@ -3402,7 +3402,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));
>
> Please add parentheses around the ?: expression to preserve the
> indentation. OK with that tweak.
Thanks, merged with the tweak as r15-7350-gd346af2af88caf.
Simon
