| Message ID | 0102019285bbcc0e-390eea2a-7e7a-494d-ae4c-53bdf9d15410-000000@eu-west-1.amazonses.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Restore rust front-end build [PR117114] | expand |
The patch failed so I’ve reverted the offending commit for the moment; commit r15-4301-ga4eec6c712ec16 refers. I’ll work on a resubmit via the initial PR (109918). Apologies for the breakage. Simon On 13 Oct 2024, at 13:55, Simon Martin wrote: > The patch that I merged via r15-4282-g60163c85730e6b breaks the build > for the rust front-end because it does not work well when virtual > inheritance is in play. > > The problem is that in such a case, an overrider and its overridden > base > method might have a different DECL_VINDEX, and the derived method > would > be incorrectly considered as hiding the base one. > > This patch fixes this by not comparing the DECL_VINDEX anymore, but > rather going back to comparing the signatures, only after having > excluded conversion operators to different types. > > I'm currently running the testsuite on x86_64-pc-linux-gnu, and > already > verified that the rust front-end builds again with the patch applied. > > OK if the testsuite run finishes successfully? > > PR c++/117114 > > gcc/cp/ChangeLog: > > * class.cc (warn_hidden): Don't compare DECL_VINDEX but > signatures, after having excluded operators to different types. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Woverloaded-virt10.C: New test. > > --- > gcc/cp/class.cc | 20 > +++++++++---------- > .../g++.dg/warn/Woverloaded-virt10.C | 7 +++++++ > 2 files changed, 17 insertions(+), 10 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Woverloaded-virt10.C > > diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc > index f754886a60a..fce71483ccc 100644 > --- a/gcc/cp/class.cc > +++ b/gcc/cp/class.cc > @@ -3279,25 +3279,25 @@ warn_hidden (tree t) > { > 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. > + /* If the methods from the base and derived classes have the > + same signature and they are not conversion operators to > + different types, then the base method has been overriden. > 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] || !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 (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 if (same_signature_p (fndecl, base_fndecls[k])) > + /* If fndecl has the same signature as base_fndecl[k], it > + overrides it. */ > + overriden_base_fndecls.add (base_fndecls[k]); > else > hidden_base_fndecls.put (base_fndecls[k], fndecl); > } > diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt10.C > b/gcc/testsuite/g++.dg/warn/Woverloaded-virt10.C > new file mode 100644 > index 00000000000..5e669360ec9 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt10.C > @@ -0,0 +1,7 @@ > +// PR c++/117114 > +// { dg-do compile { target c++11 } } > +// { dg-additional-options -Wall } > + > +struct Troops { virtual ~Troops(); }; > +struct Control { virtual int GetControl() const = 0; }; > +struct Army : Troops, Control { int GetControl() const override; }; > -- > 2.44.0
On 10/13/24 7:55 AM, Simon Martin wrote: > The patch that I merged via r15-4282-g60163c85730e6b breaks the build > for the rust front-end because it does not work well when virtual > inheritance is in play. > > The problem is that in such a case, an overrider and its overridden base > method might have a different DECL_VINDEX, and the derived method would > be incorrectly considered as hiding the base one. > > This patch fixes this by not comparing the DECL_VINDEX anymore, but > rather going back to comparing the signatures, only after having > excluded conversion operators to different types. Incidentally, it seems I was wrong to say you can just compare DECL_NAME: the name ends up being different in case of typedefs like in inherit/virtual14.C, so we do need to compare the type after all. Jason
Hi Jason, On 15 Oct 2024, at 15:18, Jason Merrill wrote: > On 10/13/24 7:55 AM, Simon Martin wrote: >> The patch that I merged via r15-4282-g60163c85730e6b breaks the build >> for the rust front-end because it does not work well when virtual >> inheritance is in play. >> >> The problem is that in such a case, an overrider and its overridden >> base >> method might have a different DECL_VINDEX, and the derived method >> would >> be incorrectly considered as hiding the base one. >> >> This patch fixes this by not comparing the DECL_VINDEX anymore, but >> rather going back to comparing the signatures, only after having >> excluded conversion operators to different types. > > Incidentally, it seems I was wrong to say you can just compare > DECL_NAME: the name ends up being different in case of typedefs like > in inherit/virtual14.C, so we do need to compare the type after all. Thanks for calling this out. I’ll integrate such a case in my next iteration. Simon
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc index f754886a60a..fce71483ccc 100644 --- a/gcc/cp/class.cc +++ b/gcc/cp/class.cc @@ -3279,25 +3279,25 @@ warn_hidden (tree t) { 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. + /* If the methods from the base and derived classes have the + same signature and they are not conversion operators to + different types, then the base method has been overriden. 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] || !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 (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 if (same_signature_p (fndecl, base_fndecls[k])) + /* If fndecl has the same signature as base_fndecl[k], it + overrides it. */ + overriden_base_fndecls.add (base_fndecls[k]); else hidden_base_fndecls.put (base_fndecls[k], fndecl); } diff --git a/gcc/testsuite/g++.dg/warn/Woverloaded-virt10.C b/gcc/testsuite/g++.dg/warn/Woverloaded-virt10.C new file mode 100644 index 00000000000..5e669360ec9 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Woverloaded-virt10.C @@ -0,0 +1,7 @@ +// PR c++/117114 +// { dg-do compile { target c++11 } } +// { dg-additional-options -Wall } + +struct Troops { virtual ~Troops(); }; +struct Control { virtual int GetControl() const = 0; }; +struct Army : Troops, Control { int GetControl() const override; };