| Message ID | 01020191ff225f9c-15dbb645-46c1-49cb-b281-1bc5d9e52c22-000000@eu-west-1.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] c++: Fix overeager Woverloaded-virtual with conversion operators [PR109918] | expand |
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. > + overriden_base_fndecls.add (base_fndecls[k]); > + else > + hidden_base_fndecls.put (base_fndecls[k], fndecl);
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc index 950d83b0ea4..ccf7a74732c 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,65 @@ 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) { + num_fns++; /* 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 + signature and return type 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. */ 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; + 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) + overriden_base_fndecls.add (base_fndecls[k]); + 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 c241c3e13bb..7963aafc841 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -3396,7 +3396,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() {} +};