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.
Jason
