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?
+ /* 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.
Jason
