| Message ID | 01020192679745e5-e6abde90-5e06-4bbc-a913-b9b3c4498991-000000@eu-west-1.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] c++: Fix overeager Woverloaded-virtual with conversion operators [PR109918] | expand |
On 10/7/24 11:27 AM, Simon Martin wrote: > 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? > + else if (IDENTIFIER_CONV_OP_P (name) > + && !same_type_p (DECL_CONV_FN_TYPE (fndecl), > + DECL_CONV_FN_TYPE (base_fndecls[k]))) You decided not to compare DECL_NAME? > /* 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? Jason
Hi Jason, On 7 Oct 2024, at 18:58, Jason Merrill wrote: > On 10/7/24 11:27 AM, Simon Martin wrote: >> 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? > >> + else if (IDENTIFIER_CONV_OP_P (name) >> + && !same_type_p (DECL_CONV_FN_TYPE (fndecl), >> + DECL_CONV_FN_TYPE (base_fndecls[k]))) > > You decided not to compare DECL_NAME? Oh I understand now: make_conv_op_name takes the “converting to” type into account, so we can just compare the DECL_NAMEs, nice! > >> /* 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. I’m attaching the (hopefully) last iteration, that compares the DECL_NAMEs for conversion operators, that I’ll be testing overnight. Thanks, Simon From bcbdcf413ecb650c717c8e49e06f93a88ce08135 Mon Sep 17 00:00:00 2001 From: Simon Martin <simon@nasilyan.com> Date: Mon, 7 Oct 2024 10:58:54 +0200 Subject: [PATCH] c++: Fix overeager Woverloaded-virtual with conversion operators [PR109918] 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 when iterating over ovl_range (fns), warn_hidden gets confused by the conversion operator marker, concludes that seen_non_override is true and therefore emits a warning for all conversion operators in A that do not convert to char, even if -Woverloaded-virtual is 1 (e.g. with -Wall, the case reported). A second set of problems is highlighted when -Woverloaded-virtual is 2. First, with the same test case, since base_fndecls contains all conversion operators in A (except the one to char, that's been removed when iterating over ovl_range (fns)), we emit a spurious warning for the conversion operator to int, even though it's unrelated. Second, in case there are several conversion operators with different cv-qualifiers to the same type in A, we rightfully emit a warning, however the note uses the location of the conversion operator marker instead of the right one; location_of should go over conv_op_marker. This patch fixes all these by explicitly keeping track of (1) base methods that are overriden, as well as (2) base methods that are hidden but not overriden (and by what), and warning about methods that are in (2) but not (1). It also ignores non virtual base methods, per "definition" of -Woverloaded-virtual. Successfully tested on x86_64-pc-linux-gnu. PR c++/109918 gcc/cp/ChangeLog: * class.cc (warn_hidden): Keep track of overloaded and of hidden base methods. Mention the actual hiding function in the warning, not the first overload. * error.cc (location_of): Skip over conv_op_marker. gcc/testsuite/ChangeLog: * g++.dg/warn/Woverloaded-virt1.C: Check that no warning is emitted for non virtual base methods. * g++.dg/warn/Woverloaded-virt5.C: New test. * g++.dg/warn/Woverloaded-virt6.C: New test. * g++.dg/warn/Woverloaded-virt7.C: New test. * g++.dg/warn/Woverloaded-virt8.C: New test. * g++.dg/warn/Woverloaded-virt9.C: New test. --- gcc/cp/class.cc | 80 ++++++++++++------- gcc/cp/error.cc | 3 +- gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C | 2 + gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C | 12 +++ gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C | 12 +++ gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C | 25 ++++++ gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C | 15 ++++ gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C | 14 ++++ 8 files changed, 133 insertions(+), 30 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc index 646072d4f20..d506f925ae2 100644 --- a/gcc/cp/class.cc +++ b/gcc/cp/class.cc @@ -3243,10 +3243,15 @@ warn_hidden (tree t) continue; tree name = OVL_NAME (fns); + size_t num_fns = 0; /* The number of fndecls in fns. */ auto_vec<tree, 20> base_fndecls; tree base_binfo; tree binfo; unsigned j; + hash_set<tree> overriden_base_fndecls; + /* base_fndecls that are hidden but not overriden. The "value" + contains the fndecl that hides the "key". */ + hash_map<tree, tree> hidden_base_fndecls; if (IDENTIFIER_CDTOR_P (name)) continue; @@ -3264,47 +3269,64 @@ warn_hidden (tree t) if (base_fndecls.is_empty ()) continue; - /* Remove any overridden functions. */ - bool seen_non_override = false; + /* Find all the base_fndecls that are overridden, as well as those + that are hidden, in T. */ for (tree fndecl : ovl_range (fns)) { - bool any_override = false; + if (TREE_CODE (fndecl) == TEMPLATE_DECL) + fndecl = DECL_TEMPLATE_RESULT (fndecl); if (TREE_CODE (fndecl) == FUNCTION_DECL - && DECL_VINDEX (fndecl)) + && fndecl != conv_op_marker) { - /* If the method from the base class has the same - signature as the method from the derived class, it - has been overridden. Note that we can't move on - after finding one match: fndecl might override - multiple base fns. */ + num_fns++; + tree fndecl_vindex = DECL_VINDEX (fndecl); + /* If the method from the base class has the same DECL_VINDEX + as the method from the derived, it has been overridden. + Note that we can't move on after finding one match: fndecl + might override multiple base fns. */ for (size_t k = 0; k < base_fndecls.length (); k++) - if (base_fndecls[k] - && same_signature_p (fndecl, base_fndecls[k])) - { - base_fndecls[k] = NULL_TREE; - any_override = true; - } + { + if (!base_fndecls[k] || !DECL_VINDEX (base_fndecls[k])) + continue; + tree base_fndecl_vindex = DECL_VINDEX (base_fndecls[k]); + if (fndecl_vindex + && !tree_int_cst_compare (fndecl_vindex, + base_fndecl_vindex)) + overriden_base_fndecls.add (base_fndecls[k]); + else if (IDENTIFIER_CONV_OP_P (name) + && (DECL_NAME (fndecl) + != DECL_NAME (base_fndecls[k]))) + /* If base_fndecl[k] and fndecl are conversion operators + to different types, they're unrelated. */ + ; + else + hidden_base_fndecls.put (base_fndecls[k], fndecl); + } } - if (!any_override) - seen_non_override = true; } - if (!seen_non_override && warn_overloaded_virtual == 1) - /* All the derived fns override base virtuals. */ - return; + if (warn_overloaded_virtual == 1 + && overriden_base_fndecls.elements () == num_fns) + /* All the fns override a base virtual. */ + continue; /* Now give a warning for all base functions without overriders, as they are hidden. */ for (tree base_fndecl : base_fndecls) - if (base_fndecl) - { - auto_diagnostic_group d; - /* Here we know it is a hider, and no overrider exists. */ - if (warning_at (location_of (base_fndecl), - OPT_Woverloaded_virtual_, - "%qD was hidden", base_fndecl)) - inform (location_of (fns), " by %qD", fns); - } + { + if (!base_fndecl || overriden_base_fndecls.contains (base_fndecl)) + continue; + tree *hider = hidden_base_fndecls.get (base_fndecl); + if (hider) + { + auto_diagnostic_group d; + /* Here we know it is a hider, and no overrider exists. */ + if (warning_at (location_of (base_fndecl), + OPT_Woverloaded_virtual_, + "%qD was hidden", base_fndecl)) + inform (location_of (*hider), " by %qD", *hider); + } + } } } diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc index 65f70c595cf..3d0dad4ee59 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -3397,7 +3397,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)); if (DECL_P (t)) return DECL_SOURCE_LOCATION (t); diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C index 92f8327b9d0..9091bfabc96 100644 --- a/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C @@ -5,10 +5,12 @@ class Foo { public: virtual void f(int); // { dg-warning "hidden" } + void g(int); // Not virtual, so no warning }; class Bar : public Foo { public: virtual void f(short); // { dg-message "by" } + virtual void g(short); }; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C new file mode 100644 index 00000000000..01cd562609a --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C @@ -0,0 +1,12 @@ +// PR c++/109918 - Exact PR testcase +// { dg-do compile } +// { dg-additional-options -Wall } + +struct A { + virtual operator int() { return 42; } + virtual operator char() = 0; +}; + +struct B : public A { + operator char() { return 'A'; } +}; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C new file mode 100644 index 00000000000..c18049e3a92 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C @@ -0,0 +1,12 @@ +// PR c++/109918 - PR testcase with -Woverloaded-virtual=2 +// { dg-do compile } +// { dg-additional-options -Woverloaded-virtual=2 } + +struct A { + virtual operator int() { return 42; } + virtual operator char() = 0; +}; + +struct B : public A { + operator char() { return 'A'; } +}; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C new file mode 100644 index 00000000000..be50c890d20 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C @@ -0,0 +1,25 @@ +// PR c++/109918 - Test different CV-quals +// { dg-do compile } +// { dg-additional-options -Woverloaded-virtual } + +struct A { + virtual operator char() { return 'a'; } + virtual operator char() const { return 'b'; } // { dg-warning "was hidden" } + virtual operator int() { return 42; } +}; + +struct B : public A { + operator char() { return 'A'; } // { dg-note "by" } + operator int() { return 43; } +}; + +struct AA { + virtual char func(char) { return 'a'; } + virtual char func(char) const { return 'b'; } // { dg-warning "was hidden" } + virtual int func(int) { return 42; } +}; + +struct BB : public AA { + char func(char) { return 'A'; } // { dg-note "by" } + int func(int) { return 43; } +}; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C new file mode 100644 index 00000000000..51af2dae77c --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C @@ -0,0 +1,15 @@ +// Identified when investigating PR c++/109918: no warning was emitted due to +// an incorrect early return in warn_hidden. +// { dg-additional-options -Wall } + +struct Foo +{ + virtual void f(int); // { dg-warning "hidden" } + virtual void g() {} +}; + +struct Bar : Foo +{ + virtual void f(short); // { dg-message "by" } + virtual void g() {} +}; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C new file mode 100644 index 00000000000..6d315c63a08 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C @@ -0,0 +1,14 @@ +// PR c++/109918: Non virtual overloads in derived classes that don't override +// anything shouldn't cause warnings, even at -Woverloaded-virtual=2 +// { dg-additional-options -Woverloaded-virtual=2 } + +struct Foo +{ + virtual void g() {} +}; + +struct Bar : Foo +{ + virtual void g() {} + void g(int) {} +};
On Mon, Oct 07, 2024 at 07:35:27PM +0000, Simon Martin wrote: > - /* Remove any overridden functions. */ > - bool seen_non_override = false; > + /* Find all the base_fndecls that are overridden, as well as those > + that are hidden, in T. */ > for (tree fndecl : ovl_range (fns)) > { > - bool any_override = false; > + if (TREE_CODE (fndecl) == TEMPLATE_DECL) > + fndecl = DECL_TEMPLATE_RESULT (fndecl); This can be fndecl = STRIP_TEMPLATE (fndecl); You don't need to repost the patch just because of this. Marek
Hi Marek, On 7 Oct 2024, at 21:44, Marek Polacek wrote: > On Mon, Oct 07, 2024 at 07:35:27PM +0000, Simon Martin wrote: >> - /* Remove any overridden functions. */ >> - bool seen_non_override = false; >> + /* Find all the base_fndecls that are overridden, as well as those >> + that are hidden, in T. */ >> for (tree fndecl : ovl_range (fns)) >> { >> - bool any_override = false; >> + if (TREE_CODE (fndecl) == TEMPLATE_DECL) >> + fndecl = DECL_TEMPLATE_RESULT (fndecl); > > This can be > fndecl = STRIP_TEMPLATE (fndecl); > You don't need to repost the patch just because of this. Indeed, thanks. Change integrated in my local branch, so that it goes in once I have an approved version. Simon
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. Or you could first iterate over overridden_base_fndecls and remove its elements from hidden_base_fndecls. Jason
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc index 646072d4f20..0854847fa03 100644 --- a/gcc/cp/class.cc +++ b/gcc/cp/class.cc @@ -3243,10 +3243,15 @@ warn_hidden (tree t) continue; tree name = OVL_NAME (fns); + size_t num_fns = 0; /* The number of fndecls in fns. */ auto_vec<tree, 20> base_fndecls; tree base_binfo; tree binfo; unsigned j; + hash_set<tree> overriden_base_fndecls; + /* base_fndecls that are hidden but not overriden. The "value" + contains the fndecl that hides the "key". */ + hash_map<tree, tree> hidden_base_fndecls; if (IDENTIFIER_CDTOR_P (name)) continue; @@ -3264,47 +3269,64 @@ warn_hidden (tree t) if (base_fndecls.is_empty ()) continue; - /* Remove any overridden functions. */ - bool seen_non_override = false; + /* Find all the base_fndecls that are overridden, as well as those + that are hidden, in T. */ for (tree fndecl : ovl_range (fns)) { - bool any_override = false; + if (TREE_CODE (fndecl) == TEMPLATE_DECL) + fndecl = DECL_TEMPLATE_RESULT (fndecl); if (TREE_CODE (fndecl) == FUNCTION_DECL - && DECL_VINDEX (fndecl)) + && fndecl != conv_op_marker) { - /* If the method from the base class has the same - signature as the method from the derived class, it - has been overridden. Note that we can't move on - after finding one match: fndecl might override - multiple base fns. */ + num_fns++; + tree fndecl_vindex = DECL_VINDEX (fndecl); + /* If the method from the base class has the same DECL_VINDEX + as the method from the derived, it has been overridden. + Note that we can't move on after finding one match: fndecl + might override multiple base fns. */ for (size_t k = 0; k < base_fndecls.length (); k++) - if (base_fndecls[k] - && same_signature_p (fndecl, base_fndecls[k])) - { - base_fndecls[k] = NULL_TREE; - any_override = true; - } + { + if (!base_fndecls[k] || !DECL_VINDEX (base_fndecls[k])) + continue; + tree base_fndecl_vindex = DECL_VINDEX (base_fndecls[k]); + if (fndecl_vindex + && !tree_int_cst_compare (fndecl_vindex, + base_fndecl_vindex)) + overriden_base_fndecls.add (base_fndecls[k]); + else if (IDENTIFIER_CONV_OP_P (name) + && !same_type_p (DECL_CONV_FN_TYPE (fndecl), + DECL_CONV_FN_TYPE (base_fndecls[k]))) + /* If base_fndecl[k] and fndecl are conversion operators + to different types, they're unrelated. */ + ; + else + hidden_base_fndecls.put (base_fndecls[k], fndecl); + } } - if (!any_override) - seen_non_override = true; } - if (!seen_non_override && warn_overloaded_virtual == 1) - /* All the derived fns override base virtuals. */ - return; + if (warn_overloaded_virtual == 1 + && overriden_base_fndecls.elements () == num_fns) + /* All the fns override a base virtual. */ + continue; /* Now give a warning for all base functions without overriders, as they are hidden. */ for (tree base_fndecl : base_fndecls) - if (base_fndecl) - { - auto_diagnostic_group d; - /* Here we know it is a hider, and no overrider exists. */ - if (warning_at (location_of (base_fndecl), - OPT_Woverloaded_virtual_, - "%qD was hidden", base_fndecl)) - inform (location_of (fns), " by %qD", fns); - } + { + if (!base_fndecl || overriden_base_fndecls.contains (base_fndecl)) + continue; + tree *hider = hidden_base_fndecls.get (base_fndecl); + if (hider) + { + auto_diagnostic_group d; + /* Here we know it is a hider, and no overrider exists. */ + if (warning_at (location_of (base_fndecl), + OPT_Woverloaded_virtual_, + "%qD was hidden", base_fndecl)) + inform (location_of (*hider), " by %qD", *hider); + } + } } } diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc index 65f70c595cf..3d0dad4ee59 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -3397,7 +3397,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)); if (DECL_P (t)) return DECL_SOURCE_LOCATION (t); diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C index 92f8327b9d0..9091bfabc96 100644 --- a/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt1.C @@ -5,10 +5,12 @@ class Foo { public: virtual void f(int); // { dg-warning "hidden" } + void g(int); // Not virtual, so no warning }; class Bar : public Foo { public: virtual void f(short); // { dg-message "by" } + virtual void g(short); }; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C new file mode 100644 index 00000000000..01cd562609a --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C @@ -0,0 +1,12 @@ +// PR c++/109918 - Exact PR testcase +// { dg-do compile } +// { dg-additional-options -Wall } + +struct A { + virtual operator int() { return 42; } + virtual operator char() = 0; +}; + +struct B : public A { + operator char() { return 'A'; } +}; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C new file mode 100644 index 00000000000..c18049e3a92 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C @@ -0,0 +1,12 @@ +// PR c++/109918 - PR testcase with -Woverloaded-virtual=2 +// { dg-do compile } +// { dg-additional-options -Woverloaded-virtual=2 } + +struct A { + virtual operator int() { return 42; } + virtual operator char() = 0; +}; + +struct B : public A { + operator char() { return 'A'; } +}; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C new file mode 100644 index 00000000000..be50c890d20 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C @@ -0,0 +1,25 @@ +// PR c++/109918 - Test different CV-quals +// { dg-do compile } +// { dg-additional-options -Woverloaded-virtual } + +struct A { + virtual operator char() { return 'a'; } + virtual operator char() const { return 'b'; } // { dg-warning "was hidden" } + virtual operator int() { return 42; } +}; + +struct B : public A { + operator char() { return 'A'; } // { dg-note "by" } + operator int() { return 43; } +}; + +struct AA { + virtual char func(char) { return 'a'; } + virtual char func(char) const { return 'b'; } // { dg-warning "was hidden" } + virtual int func(int) { return 42; } +}; + +struct BB : public AA { + char func(char) { return 'A'; } // { dg-note "by" } + int func(int) { return 43; } +}; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C new file mode 100644 index 00000000000..51af2dae77c --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C @@ -0,0 +1,15 @@ +// Identified when investigating PR c++/109918: no warning was emitted due to +// an incorrect early return in warn_hidden. +// { dg-additional-options -Wall } + +struct Foo +{ + virtual void f(int); // { dg-warning "hidden" } + virtual void g() {} +}; + +struct Bar : Foo +{ + virtual void f(short); // { dg-message "by" } + virtual void g() {} +}; diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C new file mode 100644 index 00000000000..6d315c63a08 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C @@ -0,0 +1,14 @@ +// PR c++/109918: Non virtual overloads in derived classes that don't override +// anything shouldn't cause warnings, even at -Woverloaded-virtual=2 +// { dg-additional-options -Woverloaded-virtual=2 } + +struct Foo +{ + virtual void g() {} +}; + +struct Bar : Foo +{ + virtual void g() {} + void g(int) {} +};