| Message ID | ZhkzKcoK6V49NbH8@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Diagnose or avoid constexpr dtors in classes with virtual bases [PR114426] | expand |
On 4/12/24 09:12, Jakub Jelinek wrote: > Hi! > > I had another look at this P1 PR today. > You said in the "c++: fix in-charge parm in constexpr" mail back in December > (as well as in the r14-6507 commit message): > "Since a class with vbases can't have constexpr 'tors there isn't actually > a need for an in-charge parameter in a destructor" but the ICE is because > the destructor is marked implicitly constexpr. > https://eel.is/c++draft/dcl.constexpr#3.2 says that a destructor of a class > with virtual bases is not constexpr-suitable, but we were actually > implementing this just for constructors, so clearly my fault from the > https://wg21.link/P0784R7 implementation. That paper clearly added that > sentence in there and removed similar sentence just from the constructor case. > > So, the following patch makes sure the > else if (CLASSTYPE_VBASECLASSES (DECL_CONTEXT (fun))) > { > ret = false; > if (complain) > error ("%q#T has virtual base classes", DECL_CONTEXT (fun)); > } > hunk is done no just for DECL_CONSTRUCTOR_P (fun), but also > DECL_DESTRUCTOR_P (fun) - in that case just for cxx_dialect >= cxx20, > as for cxx_dialect < cxx20 we already set ret = false; and diagnose > a different error, so no need to diagnose two. > > Bootstrapped/regtested on x86_64-linux and i686-linux, and checked it fixes > the testcase in a cross to armv7hl-linux-gnueabi, ok for trunk? OK. > 2024-04-12 Jakub Jelinek <jakub@redhat.com> > > PR c++/114426 > * constexpr.cc (is_valid_constexpr_fn): Return false/diagnose with > complain destructors in classes with virtual bases. > > * g++.dg/cpp2a/pr114426.C: New test. > * g++.dg/cpp2a/constexpr-dtor16.C: New test. > > --- gcc/cp/constexpr.cc.jj 2024-04-09 09:29:04.708521907 +0200 > +++ gcc/cp/constexpr.cc 2024-04-12 11:45:08.845476718 +0200 > @@ -262,18 +262,15 @@ is_valid_constexpr_fn (tree fun, bool co > inform (DECL_SOURCE_LOCATION (fun), > "lambdas are implicitly %<constexpr%> only in C++17 and later"); > } > - else if (DECL_DESTRUCTOR_P (fun)) > + else if (DECL_DESTRUCTOR_P (fun) && cxx_dialect < cxx20) > { > - if (cxx_dialect < cxx20) > - { > - ret = false; > - if (complain) > - error_at (DECL_SOURCE_LOCATION (fun), > - "%<constexpr%> destructors only available" > - " with %<-std=c++20%> or %<-std=gnu++20%>"); > - } > + ret = false; > + if (complain) > + error_at (DECL_SOURCE_LOCATION (fun), > + "%<constexpr%> destructors only available with " > + "%<-std=c++20%> or %<-std=gnu++20%>"); > } > - else if (!DECL_CONSTRUCTOR_P (fun)) > + else if (!DECL_CONSTRUCTOR_P (fun) && !DECL_DESTRUCTOR_P (fun)) > { > tree rettype = TREE_TYPE (TREE_TYPE (fun)); > if (!literal_type_p (rettype)) > --- gcc/testsuite/g++.dg/cpp2a/pr114426.C.jj 2024-04-12 12:05:07.443891700 +0200 > +++ gcc/testsuite/g++.dg/cpp2a/pr114426.C 2024-04-12 12:05:07.443891700 +0200 > @@ -0,0 +1,7 @@ > +// PR c++/114426 > +// { dg-do compile } > +// { dg-additional-options "-O2" } > + > +struct A { virtual ~A (); }; > +struct B : virtual A { virtual void foo () = 0; }; > +struct C : B { C () {} }; > --- gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C.jj 2024-04-12 12:05:35.398505976 +0200 > +++ gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C 2024-04-12 12:08:31.771072322 +0200 > @@ -0,0 +1,7 @@ > +// PR c++/114426 > +// { dg-do compile { target c++11 } } > + > +struct A { virtual ~A (); }; > +struct B : virtual A { constexpr ~B () {} }; > +// { dg-error "'struct B' has virtual base classes" "" { target c++20 } .-1 } > +// { dg-error "'constexpr' destructors only available with" "" { target c++17_down } .-2 } > > Jakub >
--- gcc/cp/constexpr.cc.jj 2024-04-09 09:29:04.708521907 +0200 +++ gcc/cp/constexpr.cc 2024-04-12 11:45:08.845476718 +0200 @@ -262,18 +262,15 @@ is_valid_constexpr_fn (tree fun, bool co inform (DECL_SOURCE_LOCATION (fun), "lambdas are implicitly %<constexpr%> only in C++17 and later"); } - else if (DECL_DESTRUCTOR_P (fun)) + else if (DECL_DESTRUCTOR_P (fun) && cxx_dialect < cxx20) { - if (cxx_dialect < cxx20) - { - ret = false; - if (complain) - error_at (DECL_SOURCE_LOCATION (fun), - "%<constexpr%> destructors only available" - " with %<-std=c++20%> or %<-std=gnu++20%>"); - } + ret = false; + if (complain) + error_at (DECL_SOURCE_LOCATION (fun), + "%<constexpr%> destructors only available with " + "%<-std=c++20%> or %<-std=gnu++20%>"); } - else if (!DECL_CONSTRUCTOR_P (fun)) + else if (!DECL_CONSTRUCTOR_P (fun) && !DECL_DESTRUCTOR_P (fun)) { tree rettype = TREE_TYPE (TREE_TYPE (fun)); if (!literal_type_p (rettype)) --- gcc/testsuite/g++.dg/cpp2a/pr114426.C.jj 2024-04-12 12:05:07.443891700 +0200 +++ gcc/testsuite/g++.dg/cpp2a/pr114426.C 2024-04-12 12:05:07.443891700 +0200 @@ -0,0 +1,7 @@ +// PR c++/114426 +// { dg-do compile } +// { dg-additional-options "-O2" } + +struct A { virtual ~A (); }; +struct B : virtual A { virtual void foo () = 0; }; +struct C : B { C () {} }; --- gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C.jj 2024-04-12 12:05:35.398505976 +0200 +++ gcc/testsuite/g++.dg/cpp2a/constexpr-dtor16.C 2024-04-12 12:08:31.771072322 +0200 @@ -0,0 +1,7 @@ +// PR c++/114426 +// { dg-do compile { target c++11 } } + +struct A { virtual ~A (); }; +struct B : virtual A { constexpr ~B () {} }; +// { dg-error "'struct B' has virtual base classes" "" { target c++20 } .-1 } +// { dg-error "'constexpr' destructors only available with" "" { target c++17_down } .-2 }
Hi! I had another look at this P1 PR today. You said in the "c++: fix in-charge parm in constexpr" mail back in December (as well as in the r14-6507 commit message): "Since a class with vbases can't have constexpr 'tors there isn't actually a need for an in-charge parameter in a destructor" but the ICE is because the destructor is marked implicitly constexpr. https://eel.is/c++draft/dcl.constexpr#3.2 says that a destructor of a class with virtual bases is not constexpr-suitable, but we were actually implementing this just for constructors, so clearly my fault from the https://wg21.link/P0784R7 implementation. That paper clearly added that sentence in there and removed similar sentence just from the constructor case. So, the following patch makes sure the else if (CLASSTYPE_VBASECLASSES (DECL_CONTEXT (fun))) { ret = false; if (complain) error ("%q#T has virtual base classes", DECL_CONTEXT (fun)); } hunk is done no just for DECL_CONSTRUCTOR_P (fun), but also DECL_DESTRUCTOR_P (fun) - in that case just for cxx_dialect >= cxx20, as for cxx_dialect < cxx20 we already set ret = false; and diagnose a different error, so no need to diagnose two. Bootstrapped/regtested on x86_64-linux and i686-linux, and checked it fixes the testcase in a cross to armv7hl-linux-gnueabi, ok for trunk? 2024-04-12 Jakub Jelinek <jakub@redhat.com> PR c++/114426 * constexpr.cc (is_valid_constexpr_fn): Return false/diagnose with complain destructors in classes with virtual bases. * g++.dg/cpp2a/pr114426.C: New test. * g++.dg/cpp2a/constexpr-dtor16.C: New test. Jakub