| Message ID | 010201918020573c-e8ed8ba3-c58a-4f09-a458-929c10e5c119-000000@eu-west-1.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Fix overeager Woverloaded-virtual with conversion operators [PR109918] | expand |
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? > Successfully tested on x86_64-pc-linux-gnu. > > PR c++/109918 > > gcc/cp/ChangeLog: > > * class.cc (warn_hidden): Filter out conversion operators to different > types. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Woverloaded-virt5.C: New test. > > --- > gcc/cp/class.cc | 33 ++++++++++++------- > gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C | 12 +++++++ > 2 files changed, 34 insertions(+), 11 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C > > diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc > index fb6c3370950..a8178a31fe8 100644 > --- a/gcc/cp/class.cc > +++ b/gcc/cp/class.cc > @@ -3267,18 +3267,29 @@ warn_hidden (tree t) > if (TREE_CODE (fndecl) == FUNCTION_DECL > && DECL_VINDEX (fndecl)) > { > - /* 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. */ > 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]) > + continue; > + /* If FNS is a conversion operator, base_fndecls contains > + all conversion operators from base classes; we need to > + remove those converting to a different type. */ > + 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; > + } > + /* If the method from the base class has the same signature > + as the method from the derived class, it has been > + overriden. Note that we can't move on after finding > + one match: fndecl might override multiple base fns. */ > + else if (same_signature_p (fndecl, base_fndecls[k])) > + { > + base_fndecls[k] = NULL_TREE; > + any_override = true; > + } > + } > } > if (!any_override) > seen_non_override = true; > 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..ea26569e565 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C > @@ -0,0 +1,12 @@ > +// PR c++/109918 > +// { dg-do compile } > +// { dg-additional-options -Woverloaded-virtual } > + > +struct A { > + virtual operator int() { return 42; } > + virtual operator char() = 0; > +}; > + > +struct B : public A { > + operator char() { return 'A'; } > +};
Hi Jason, 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. 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. Thanks! Simon >> Successfully tested on x86_64-pc-linux-gnu. >> >> PR c++/109918 >> >> gcc/cp/ChangeLog: >> >> * class.cc (warn_hidden): Filter out conversion operators to >> different >> types. >> >> gcc/testsuite/ChangeLog: >> >> * g++.dg/warn/Woverloaded-virt5.C: New test. >> >> --- >> gcc/cp/class.cc | 33 >> ++++++++++++------- >> gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C | 12 +++++++ >> 2 files changed, 34 insertions(+), 11 deletions(-) >> create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C >> >> diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc >> index fb6c3370950..a8178a31fe8 100644 >> --- a/gcc/cp/class.cc >> +++ b/gcc/cp/class.cc >> @@ -3267,18 +3267,29 @@ warn_hidden (tree t) >> if (TREE_CODE (fndecl) == FUNCTION_DECL >> && DECL_VINDEX (fndecl)) >> { >> - /* 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. */ >> 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]) >> + continue; >> + /* If FNS is a conversion operator, base_fndecls contains >> + all conversion operators from base classes; we need to >> + remove those converting to a different type. */ >> + 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; >> + } >> + /* If the method from the base class has the same signature >> + as the method from the derived class, it has been >> + overriden. Note that we can't move on after finding >> + one match: fndecl might override multiple base fns. */ >> + else if (same_signature_p (fndecl, base_fndecls[k])) >> + { >> + base_fndecls[k] = NULL_TREE; >> + any_override = true; >> + } >> + } >> } >> if (!any_override) >> seen_non_override = true; >> 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..ea26569e565 >> --- /dev/null >> +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C >> @@ -0,0 +1,12 @@ >> +// PR c++/109918 >> +// { dg-do compile } >> +// { dg-additional-options -Woverloaded-virtual } >> + >> +struct A { >> + virtual operator int() { return 42; } >> + virtual operator char() = 0; >> +}; >> + >> +struct B : public A { >> + operator char() { return 'A'; } >> +};
Hi Jason, On 24 Aug 2024, at 23:59, Simon Martin wrote: > Hi Jason, > > 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. > > 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. 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. Successfully tested on x86_64-pc-linux-gnu; OK for trunk? Thanks! Simon > >>> Successfully tested on x86_64-pc-linux-gnu. >>> >>> PR c++/109918 >>> >>> gcc/cp/ChangeLog: >>> >>> * class.cc (warn_hidden): Filter out conversion operators to >>> different >>> types. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * g++.dg/warn/Woverloaded-virt5.C: New test. >>> >>> --- >>> gcc/cp/class.cc | 33 >>> ++++++++++++------- >>> gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C | 12 +++++++ >>> 2 files changed, 34 insertions(+), 11 deletions(-) >>> create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C >>> >>> diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc >>> index fb6c3370950..a8178a31fe8 100644 >>> --- a/gcc/cp/class.cc >>> +++ b/gcc/cp/class.cc >>> @@ -3267,18 +3267,29 @@ warn_hidden (tree t) >>> if (TREE_CODE (fndecl) == FUNCTION_DECL >>> && DECL_VINDEX (fndecl)) >>> { >>> - /* 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. */ >>> 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]) >>> + continue; >>> + /* If FNS is a conversion operator, base_fndecls contains >>> + all conversion operators from base classes; we need to >>> + remove those converting to a different type. */ >>> + 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; >>> + } >>> + /* If the method from the base class has the same signature >>> + as the method from the derived class, it has been >>> + overriden. Note that we can't move on after finding >>> + one match: fndecl might override multiple base fns. */ >>> + else if (same_signature_p (fndecl, base_fndecls[k])) >>> + { >>> + base_fndecls[k] = NULL_TREE; >>> + any_override = true; >>> + } >>> + } >>> } >>> if (!any_override) >>> seen_non_override = true; >>> 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..ea26569e565 >>> --- /dev/null >>> +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C >>> @@ -0,0 +1,12 @@ >>> +// PR c++/109918 >>> +// { dg-do compile } >>> +// { dg-additional-options -Woverloaded-virtual } >>> + >>> +struct A { >>> + virtual operator int() { return 42; } >>> + virtual operator char() = 0; >>> +}; >>> + >>> +struct B : public A { >>> + operator char() { return 'A'; } >>> +}; From 5efe3910b4cd98a6565cdea4a977f9349a1d871f Mon Sep 17 00:00:00 2001 From: Simon Martin <simon@nasilyan.com> Date: Sun, 25 Aug 2024 15:17:47 +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 first 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 not overriden. The patch fixes both this and the first problem by nullifying conversion operators to different types in base_fndecls, and validates both fixes via Woverloaded-virt5.C and Woverloaded-virt6.C. 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. The patch fixes this by going over conv_op_marker in location_of, and validates the fix via Woverloaded-virt7.C. Successfully tested on x86_64-pc-linux-gnu. PR c++/109918 gcc/cp/ChangeLog: * class.cc (warn_hidden): Ignore conversion operators to different types. * error.cc (location_of): Skip over conv_op_marker. gcc/testsuite/ChangeLog: * g++.dg/warn/Woverloaded-virt5.C: New test. * g++.dg/warn/Woverloaded-virt6.C: New test. * g++.dg/warn/Woverloaded-virt7.C: New test. --- gcc/cp/class.cc | 28 ++++++++++++++----- gcc/cp/error.cc | 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 | 12 ++++++++ 5 files changed, 58 insertions(+), 8 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 diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc index fb6c3370950..bf7dfc5a8cf 100644 --- a/gcc/cp/class.cc +++ b/gcc/cp/class.cc @@ -3271,14 +3271,28 @@ warn_hidden (tree t) 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. */ + multiple base fns. + If the signatures are different and the method is a + conversion operator, the base and derived methods + might convert to a different type. In this case, the + base one should be ignored. */ 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]) + continue; + 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; + } + else if (same_signature_p (fndecl, base_fndecls[k])) + { + base_fndecls[k] = NULL_TREE; + any_override = true; + } + } } if (!any_override) seen_non_override = true; diff --git a/gcc/cp/error.cc b/gcc/cp/error.cc index 879e5a115cf..c0c8df31db8 100644 --- a/gcc/cp/error.cc +++ b/gcc/cp/error.cc @@ -3366,7 +3366,7 @@ 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_CHAIN (t); if (DECL_P (t)) return DECL_SOURCE_LOCATION (t); 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..0c954b1930f --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C @@ -0,0 +1,12 @@ +// PR c++/109918 +// { dg-do compile } +// { dg-additional-options -Woverloaded-virtual=1 } + +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..b99b9933688 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C @@ -0,0 +1,12 @@ +// PR c++/109918 +// { 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..8f47e080f8c --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C @@ -0,0 +1,12 @@ +// PR c++/109918 +// { 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" } +}; + +struct B : public A { + operator char() { return 'A'; } // { dg-note "by" } +};
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. > else if (TREE_CODE (t) == OVERLOAD) > + t = OVL_FIRST (t) != conv_op_marker ? OVL_FIRST (t) : OVL_CHAIN (t); Usually OVL_CHAIN will be another OVERLOAD, you want OVL_FIRST (OVL_CHAIN (t)) in that case. Jason
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) - an invalid early return - see Woverloaded-virt9.C, - reporting the first overload instead of the one that actually hides - see the dg-note in Woverloaded-virt8.C that’d fail without the patch because we’d report the int overload) Successfully tested on x86_64-pc-linux-gnu; OK for trunk? >> else if (TREE_CODE (t) == OVERLOAD) >> + t = OVL_FIRST (t) != conv_op_marker ? OVL_FIRST (t) : OVL_CHAIN >> (t); > > Usually OVL_CHAIN will be another OVERLOAD, you want OVL_FIRST > (OVL_CHAIN (t)) in that case. Thanks. Even though not strictly needed anymore by the updated patch, I’m still including the (fixed) change in the patch. Simon From 2dd66681842725649abdaef0eec3df553264fb80 Mon Sep 17 00:00:00 2001 From: Simon Martin <simon@nasilyan.com> Date: Sun, 25 Aug 2024 15:17:47 +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 not overriden. 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/pr61945.C: Adjust test expectations, since template methods don't override 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 | 68 ++++++++++++------- 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 | 21 ++++++ gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C | 25 +++++++ gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C | 15 ++++ gcc/testsuite/g++.dg/warn/pr61945.C | 6 +- 9 files changed, 137 insertions(+), 27 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 fb6c3370950..da214b7af53 100644 --- a/gcc/cp/class.cc +++ b/gcc/cp/class.cc @@ -3238,10 +3238,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; @@ -3259,47 +3264,62 @@ 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) == 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 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; + else if (same_signature_p (fndecl, base_fndecls[k])) + overriden_base_fndecls.add (base_fndecls[k]); + else if (IDENTIFIER_CONV_OP_P (name)) + { + if (same_type_p (DECL_CONV_FN_TYPE (fndecl), + DECL_CONV_FN_TYPE (base_fndecls[k]))) + /* fndecl and base_fndecls[k] convert to the same + type, but have different CV qualifiers, so fndecl + hides base_fndecl[k]. */ + hidden_base_fndecls.put (base_fndecls[k], fndecl); + } + 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 420fad26b7b..eb059c3b451 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..0c954b1930f --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C @@ -0,0 +1,12 @@ +// PR c++/109918 +// { dg-do compile } +// { dg-additional-options -Woverloaded-virtual=1 } + +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..b99b9933688 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt6.C @@ -0,0 +1,12 @@ +// PR c++/109918 +// { 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..ece948fff72 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt7.C @@ -0,0 +1,21 @@ +// PR c++/109918 +// { 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" } +}; + +struct B : public A { + operator char() { return 'A'; } // { dg-note "by" } +}; + +struct AA { + virtual char func(char) { return 'a'; } + virtual char func(char) const { return 'b'; } // { dg-warning "was hidden" } +}; + +struct BB : public AA { + char func(char) { return 'A'; } // { dg-note "by" } +}; 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..5fec6ee5c57 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt8.C @@ -0,0 +1,25 @@ +// PR c++/109918 +// { dg-do compile } +// { dg-additional-options -Woverloaded-virtual=2 } + +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-virt9.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.C new file mode 100644 index 00000000000..51af2dae77c --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt9.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/pr61945.C b/gcc/testsuite/g++.dg/warn/pr61945.C index 2252330835f..2dc34bcc475 100644 --- a/gcc/testsuite/g++.dg/warn/pr61945.C +++ b/gcc/testsuite/g++.dg/warn/pr61945.C @@ -2,10 +2,12 @@ // { dg-do compile } // { dg-options "-Woverloaded-virtual" } +// No warning because template members don't override virtual base methods + class A { - virtual int foo (); // { dg-warning "was hidden" } + virtual int foo (); }; class B : A { template <typename> - void foo (); // { dg-message "by .*B::foo\\(\\)." } + int foo (int); };
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. > - an invalid early return - see Woverloaded-virt9.C, > - reporting the first overload instead of the one that actually hides - > see the dg-note in Woverloaded-virt8.C that’d fail without the patch > because we’d report the int overload) > > Successfully tested on x86_64-pc-linux-gnu; OK for trunk? > >>> else if (TREE_CODE (t) == OVERLOAD) >>> + t = OVL_FIRST (t) != conv_op_marker ? OVL_FIRST (t) : OVL_CHAIN >>> (t); >> >> Usually OVL_CHAIN will be another OVERLOAD, you want OVL_FIRST >> (OVL_CHAIN (t)) in that case. > Thanks. Even though not strictly needed anymore by the updated patch, > > I’m still including the (fixed) change in the patch. > > Simon
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). Successfully tested on x86_64-pc-linux-gnu; OK for trunk? Thanks, Simon > >> - an invalid early return - see Woverloaded-virt9.C, >> - reporting the first overload instead of the one that actually hides >> - >> see the dg-note in Woverloaded-virt8.C that’d fail without the >> patch >> because we’d report the int overload) >> >> Successfully tested on x86_64-pc-linux-gnu; OK for trunk? >> >>>> else if (TREE_CODE (t) == OVERLOAD) >>>> + t = OVL_FIRST (t) != conv_op_marker ? OVL_FIRST (t) : >>>> OVL_CHAIN >>>> (t); >>> >>> Usually OVL_CHAIN will be another OVERLOAD, you want OVL_FIRST >>> (OVL_CHAIN (t)) in that case. >> Thanks. Even though not strictly needed anymore by the updated patch, >> >> I’m still including the (fixed) change in the patch. >> >> Simon From 40ef56e5d1f56b2176d87e7fec5a2a51780351b2 Mon Sep 17 00:00:00 2001 From: Simon Martin <simon@nasilyan.com> Date: Sun, 25 Aug 2024 15:17:47 +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 not overriden. 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/pr61945.C: Change signature to really hide base method. * 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. --- gcc/cp/class.cc | 70 ++++++++++++------- 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/pr61945.C | 2 +- 8 files changed, 115 insertions(+), 26 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 diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc index fb6c3370950..850a2f14a03 100644 --- a/gcc/cp/class.cc +++ b/gcc/cp/class.cc @@ -3238,10 +3238,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; @@ -3259,47 +3264,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) { + 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 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; + else if (same_signature_p (fndecl, base_fndecls[k])) + overriden_base_fndecls.add (base_fndecls[k]); + else if (IDENTIFIER_CONV_OP_P (name)) + { + if (same_type_p (DECL_CONV_FN_TYPE (fndecl), + DECL_CONV_FN_TYPE (base_fndecls[k]))) + /* fndecl and base_fndecls[k] convert to the same + type, but have different CV qualifiers, so fndecl + hides base_fndecl[k]. */ + hidden_base_fndecls.put (base_fndecls[k], fndecl); + } + 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 4a9e9aa3cdc..401dbb43e56 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() {} +}; diff --git a/gcc/testsuite/g++.dg/warn/pr61945.C b/gcc/testsuite/g++.dg/warn/pr61945.C index 2252330835f..f7b1f5fc442 100644 --- a/gcc/testsuite/g++.dg/warn/pr61945.C +++ b/gcc/testsuite/g++.dg/warn/pr61945.C @@ -7,5 +7,5 @@ class A { }; class B : A { template <typename> - void foo (); // { dg-message "by .*B::foo\\(\\)." } + void foo (int); // { dg-message "by .*B::foo\\(int\\)." } };
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. Jason
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc index fb6c3370950..a8178a31fe8 100644 --- a/gcc/cp/class.cc +++ b/gcc/cp/class.cc @@ -3267,18 +3267,29 @@ warn_hidden (tree t) if (TREE_CODE (fndecl) == FUNCTION_DECL && DECL_VINDEX (fndecl)) { - /* 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. */ 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]) + continue; + /* If FNS is a conversion operator, base_fndecls contains + all conversion operators from base classes; we need to + remove those converting to a different type. */ + 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; + } + /* If the method from the base class has the same signature + as the method from the derived class, it has been + overriden. Note that we can't move on after finding + one match: fndecl might override multiple base fns. */ + else if (same_signature_p (fndecl, base_fndecls[k])) + { + base_fndecls[k] = NULL_TREE; + any_override = true; + } + } } if (!any_override) seen_non_override = true; 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..ea26569e565 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C @@ -0,0 +1,12 @@ +// PR c++/109918 +// { dg-do compile } +// { dg-additional-options -Woverloaded-virtual } + +struct A { + virtual operator int() { return 42; } + virtual operator char() = 0; +}; + +struct B : public A { + operator char() { return 'A'; } +};
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. Successfully tested on x86_64-pc-linux-gnu. PR c++/109918 gcc/cp/ChangeLog: * class.cc (warn_hidden): Filter out conversion operators to different types. gcc/testsuite/ChangeLog: * g++.dg/warn/Woverloaded-virt5.C: New test. --- gcc/cp/class.cc | 33 ++++++++++++------- gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C | 12 +++++++ 2 files changed, 34 insertions(+), 11 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt5.C