| Message ID | CAFk2RUYfaVBdJD-AJw1G0BjMas1m=R3mD0_x6B56WObPccY6mQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] PR c++/87051 | expand |
On 13 September 2018 at 10:38, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > Howdy! The tricky details of copy constructors, part 76. > In this approach, I let the const-copy "dominate"; that is, if > a const-copy was found, a non-const-copy will not turn off triviality > of the copy constructor, and a const-copy will reinstate triviality > of the copy constructor even if a non-const copy removed said > triviality. Curses.. the resetting is over-eager; we might have a non-trivial base or a member, and in those cases we shouldn't reset the triviality when we see a non-user-provided const copy. I think I'll hack around this with a non 0/1 value. :)
On 13 September 2018 at 12:08, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > Curses.. the resetting is over-eager; we might have a non-trivial base > or a member, and in those cases we shouldn't > reset the triviality when we see a non-user-provided const copy. I > think I'll hack around this with a non 0/1 value. :) Testing the attached. I think it might need a comment, and I'm not sure how to word it. Here are some options: 1) /* Yo dawg, we heard you'd like another bit, so we added a second bit to your bit so you can read the other bit when you read your bit */ 2) /* I'm vewy vewy sowwy */ 3) /* We need to attach another bit to the TYPE_HAS_COMPLEX_COPY_CTOR bit, so let's quantum-entangle the additional bit and the actual bit */ diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 50b60e8..ce6ac7f 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -13212,7 +13212,14 @@ grok_special_member_properties (tree decl) default arguments. */ TYPE_HAS_COPY_CTOR (class_type) = 1; if (user_provided_p (decl)) - TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 1; + { + if (ctor > 1) + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 1; + else if (!TYPE_HAS_CONST_COPY_CTOR (class_type)) + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) += 2; + } + else if (ctor > 1 && TYPE_HAS_COMPLEX_COPY_CTOR (class_type) == 2) + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 0; if (ctor > 1) TYPE_HAS_CONST_COPY_CTOR (class_type) = 1; } diff --git a/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C b/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C new file mode 100644 index 0000000..177eb2e --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C @@ -0,0 +1,16 @@ +// { dg-do compile { target c++11 } } + +// PR c++/87051 + +struct M { + M(const M&) = default; + M(M&); +}; + +struct M2 { + M2(M2&); + M2(const M2&) = default; +}; + +static_assert( __is_trivially_constructible(M, M&&), ""); +static_assert( __is_trivially_constructible(M2, M2&&), "");
On Thu, Sep 13, 2018 at 12:56:59PM +0300, Ville Voutilainen wrote: > + else if (!TYPE_HAS_CONST_COPY_CTOR (class_type)) > + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) += 2; > + } > + else if (ctor > 1 && TYPE_HAS_COMPLEX_COPY_CTOR (class_type) == 2) > + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 0; How does this work when: unsigned has_complex_copy_ctor : 1; ... #define TYPE_HAS_COMPLEX_COPY_CTOR(NODE) (LANG_TYPE_CLASS_CHECK (NODE)->has_complex_copy_ctor) ? > if (ctor > 1) > TYPE_HAS_CONST_COPY_CTOR (class_type) = 1; Jakub
On 13 September 2018 at 13:03, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Sep 13, 2018 at 12:56:59PM +0300, Ville Voutilainen wrote: >> + else if (!TYPE_HAS_CONST_COPY_CTOR (class_type)) >> + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) += 2; >> + } >> + else if (ctor > 1 && TYPE_HAS_COMPLEX_COPY_CTOR (class_type) == 2) >> + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 0; > > How does this work when: > unsigned has_complex_copy_ctor : 1; It doesn't. I need more bits. Luckily, we have some available.
On 13 September 2018 at 13:41, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: >> How does this work when: >> unsigned has_complex_copy_ctor : 1; > It doesn't. I need more bits. Luckily, we have some available. Here. I suppose this could theoretically be done in some later stage of class completion, but that would presumably be an additional member function loop that looks at the constructors, weeds out copy constructors, and calculates the triviality bit (and it should probably then also look at fields and bases, again). So while I continue to have a minor distaste for this whole approach, and how it wastes two perfectly good bits for a dark corner case, I think I can learn to live with it. Tests pass on Linux-PPC64, although the suite is still running some library tests, so I think I'll do a paranoid re-run. OK for trunk? I'm not planning to backport this, it's not a regression. 2018-09-13 Ville Voutilainen <ville.voutilainen@gmail.com> gcc/cp PR c++/87051 * cp-tree.h (lang_type): Grow has_complex_copy_ctor and shrink dummy. * decl.c (grok_special_member_properties): Don't mark the class as having a non-trivial copy constructor if the copy constructor we're looking at is not a const-copy and a const-copy was already found, reset the copy constructor triviality if we found a trivial const-copy. testsuite/ PR c++/87051 * g++.dg/ext/is_trivially_constructible7.C: New. diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 6cd6e5f..fa39f6a 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -2046,7 +2046,7 @@ struct GTY(()) lang_type { unsigned lazy_copy_assign : 1; unsigned lazy_destructor : 1; unsigned has_const_copy_ctor : 1; - unsigned has_complex_copy_ctor : 1; + unsigned has_complex_copy_ctor : 2; unsigned has_complex_copy_assign : 1; unsigned non_aggregate : 1; @@ -2070,7 +2070,7 @@ struct GTY(()) lang_type { /* There are some bits left to fill out a 32-bit word. Keep track of this by updating the size of this bitfield whenever you add or remove a flag. */ - unsigned dummy : 4; + unsigned dummy : 3; tree primary_base; vec<tree_pair_s, va_gc> *vcall_indices; diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 50b60e8..452e968 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -13212,7 +13212,15 @@ grok_special_member_properties (tree decl) default arguments. */ TYPE_HAS_COPY_CTOR (class_type) = 1; if (user_provided_p (decl)) - TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 1; + { + if (ctor > 1) + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 1; + else if (!TYPE_HAS_CONST_COPY_CTOR (class_type) + && TYPE_HAS_COMPLEX_COPY_CTOR (class_type) < 2) + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) += 2; + } + else if (ctor > 1 && TYPE_HAS_COMPLEX_COPY_CTOR (class_type) == 2) + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 0; if (ctor > 1) TYPE_HAS_CONST_COPY_CTOR (class_type) = 1; } diff --git a/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C b/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C new file mode 100644 index 0000000..177eb2e --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C @@ -0,0 +1,16 @@ +// { dg-do compile { target c++11 } } + +// PR c++/87051 + +struct M { + M(const M&) = default; + M(M&); +}; + +struct M2 { + M2(M2&); + M2(const M2&) = default; +}; + +static_assert( __is_trivially_constructible(M, M&&), ""); +static_assert( __is_trivially_constructible(M2, M2&&), "");
Hi! On Thu, Sep 13, 2018 at 04:41:25PM +0300, Ville Voutilainen wrote: > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -2046,7 +2046,7 @@ struct GTY(()) lang_type { > unsigned lazy_copy_assign : 1; > unsigned lazy_destructor : 1; > unsigned has_const_copy_ctor : 1; > - unsigned has_complex_copy_ctor : 1; > + unsigned has_complex_copy_ctor : 2; > unsigned has_complex_copy_assign : 1; > > unsigned non_aggregate : 1; Just a formatting nit, there is currently an empty new-line after each group of 8 bits, so if you add one bit in the middle, the empty lines need to move as well (i.e. empty line before has_complex_copy_assign instead of after it and ditto for has_complex_move_ctor). I'll defer actual review to Jason/Nathan. > @@ -2070,7 +2070,7 @@ struct GTY(()) lang_type { > /* There are some bits left to fill out a 32-bit word. Keep track > of this by updating the size of this bitfield whenever you add or > remove a flag. */ > - unsigned dummy : 4; > + unsigned dummy : 3; > > tree primary_base; > vec<tree_pair_s, va_gc> *vcall_indices; Jakub
On Thu, Sep 13, 2018 at 9:41 AM, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > On 13 September 2018 at 13:41, Ville Voutilainen > <ville.voutilainen@gmail.com> wrote: >>> How does this work when: >>> unsigned has_complex_copy_ctor : 1; >> It doesn't. I need more bits. Luckily, we have some available. > > Here. I suppose this could theoretically be done in some later stage > of class completion, > but that would presumably be an additional member function loop that > looks at the constructors, > weeds out copy constructors, and calculates the triviality bit (and it > should probably then also > look at fields and bases, again). So while I continue to have a minor > distaste for this whole approach, > and how it wastes two perfectly good bits for a dark corner case, I > think I can learn to live with it. Really, the problem is that trivial_fn_p shouldn't use type_has_trivial_fn, and also that the function named "type_has_trivial_fn" actually returns "type has no non-trivial fn". These flags are relics of C++98 semantics. Your test should also check that !__is_trivially_constructible(M,M&) and !__is_trivially_constructible(M2,M2&). I suppose that given the limited number of possibly trivial signatures, we can still use flag bits on the class, as your patch is heading toward: one bit for each of the possibly trivial signatures, and TYPE_HAS_COMPLEX_COPY_CTOR; then TYPE_HAS_COPY_CTOR is the union of these. And similarly for the other possibly trivial functions. Jason
On 13 September 2018 at 19:36, Jason Merrill <jason@redhat.com> wrote: > On Thu, Sep 13, 2018 at 9:41 AM, Ville Voutilainen > <ville.voutilainen@gmail.com> wrote: >> On 13 September 2018 at 13:41, Ville Voutilainen >> <ville.voutilainen@gmail.com> wrote: >>>> How does this work when: >>>> unsigned has_complex_copy_ctor : 1; >>> It doesn't. I need more bits. Luckily, we have some available. >> >> Here. I suppose this could theoretically be done in some later stage >> of class completion, >> but that would presumably be an additional member function loop that >> looks at the constructors, >> weeds out copy constructors, and calculates the triviality bit (and it >> should probably then also >> look at fields and bases, again). So while I continue to have a minor >> distaste for this whole approach, >> and how it wastes two perfectly good bits for a dark corner case, I >> think I can learn to live with it. > > Really, the problem is that trivial_fn_p shouldn't use > type_has_trivial_fn, and also that the function named > "type_has_trivial_fn" actually returns "type has no non-trivial fn". > These flags are relics of C++98 semantics. Your test should also > check that !__is_trivially_constructible(M,M&) and > !__is_trivially_constructible(M2,M2&). Yeah, this patch doesn't handle those trivialities correctly, that needs further work. > I suppose that given the limited number of possibly trivial > signatures, we can still use flag bits on the class, as your patch is > heading toward: one bit for each of the possibly trivial signatures, > and TYPE_HAS_COMPLEX_COPY_CTOR; then TYPE_HAS_COPY_CTOR is the union > of these. And similarly for the other possibly trivial functions. Okay. Do you think we should have an sfk_kind for non-canonical copy/move operations? That would presumably make it a tad more straightforward to go from fndecl to whatever class bits, instead of what's currently there, where we say "yeah I had a fndecl, now I turned it into an sfk_kind that says it's a copy constructor, but guess which one when you're deeming its triviality". ;)
On Thu, Sep 13, 2018 at 12:42 PM, Ville Voutilainen <ville.voutilainen@gmail.com> wrote: > On 13 September 2018 at 19:36, Jason Merrill <jason@redhat.com> wrote: >> On Thu, Sep 13, 2018 at 9:41 AM, Ville Voutilainen >> <ville.voutilainen@gmail.com> wrote: >>> On 13 September 2018 at 13:41, Ville Voutilainen >>> <ville.voutilainen@gmail.com> wrote: >>>>> How does this work when: >>>>> unsigned has_complex_copy_ctor : 1; >>>> It doesn't. I need more bits. Luckily, we have some available. >>> >>> Here. I suppose this could theoretically be done in some later stage >>> of class completion, >>> but that would presumably be an additional member function loop that >>> looks at the constructors, >>> weeds out copy constructors, and calculates the triviality bit (and it >>> should probably then also >>> look at fields and bases, again). So while I continue to have a minor >>> distaste for this whole approach, >>> and how it wastes two perfectly good bits for a dark corner case, I >>> think I can learn to live with it. >> >> Really, the problem is that trivial_fn_p shouldn't use >> type_has_trivial_fn, and also that the function named >> "type_has_trivial_fn" actually returns "type has no non-trivial fn". >> These flags are relics of C++98 semantics. Your test should also >> check that !__is_trivially_constructible(M,M&) and >> !__is_trivially_constructible(M2,M2&). > > Yeah, this patch doesn't handle those trivialities correctly, that > needs further work. > >> I suppose that given the limited number of possibly trivial >> signatures, we can still use flag bits on the class, as your patch is >> heading toward: one bit for each of the possibly trivial signatures, >> and TYPE_HAS_COMPLEX_COPY_CTOR; then TYPE_HAS_COPY_CTOR is the union >> of these. And similarly for the other possibly trivial functions. > > Okay. Do you think we should have an sfk_kind for non-canonical > copy/move operations? That would presumably make it a tad more straightforward to go from > fndecl to whatever class bits, instead of what's currently there, where we say "yeah I had a fndecl, > now I turned it into an sfk_kind that says it's a copy constructor, but guess which one when you're > deeming its triviality". ;) I suppose it would be possible to have a more detailed sfk_kind for distinguishing between different signatures, but I'm inclined instead to stop using sfk_kind in trivial_fn_p. Even if having an enumeration is convenient for dispatch (or bitmapping), it doesn't need to be the same enum. Jason
On 13 September 2018 at 20:41, Jason Merrill <jason@redhat.com> wrote: >> Okay. Do you think we should have an sfk_kind for non-canonical >> copy/move operations? That would presumably make it a tad more straightforward to go from >> fndecl to whatever class bits, instead of what's currently there, where we say "yeah I had a fndecl, >> now I turned it into an sfk_kind that says it's a copy constructor, but guess which one when you're >> deeming its triviality". ;) > > I suppose it would be possible to have a more detailed sfk_kind for > distinguishing between different signatures, but I'm inclined instead > to stop using sfk_kind in trivial_fn_p. Even if having an enumeration > is convenient for dispatch (or bitmapping), it doesn't need to be the > same enum. Yeah, the idea of using a different enum dawned on me straight after sending that email. ;) I'll give this approach a spin, more bits into the lang_type and a different mapping, that way we should indeed get correct answers for all cases.
On Thu, Sep 13, 2018 at 08:58:34PM +0300, Ville Voutilainen wrote: > On 13 September 2018 at 20:41, Jason Merrill <jason@redhat.com> wrote: > >> Okay. Do you think we should have an sfk_kind for non-canonical > >> copy/move operations? That would presumably make it a tad more straightforward to go from > >> fndecl to whatever class bits, instead of what's currently there, where we say "yeah I had a fndecl, > >> now I turned it into an sfk_kind that says it's a copy constructor, but guess which one when you're > >> deeming its triviality". ;) > > > > I suppose it would be possible to have a more detailed sfk_kind for > > distinguishing between different signatures, but I'm inclined instead > > to stop using sfk_kind in trivial_fn_p. Even if having an enumeration > > is convenient for dispatch (or bitmapping), it doesn't need to be the > > same enum. > > Yeah, the idea of using a different enum dawned on me straight after > sending that email. ;) > I'll give this approach a spin, more bits into the lang_type and a > different mapping, that way we should indeed > get correct answers for all cases. Hi Ville, any updates? Marek
On Tue, 11 Dec 2018 at 20:58, Marek Polacek <polacek@redhat.com> wrote: > > On Thu, Sep 13, 2018 at 08:58:34PM +0300, Ville Voutilainen wrote: > > On 13 September 2018 at 20:41, Jason Merrill <jason@redhat.com> wrote: > > >> Okay. Do you think we should have an sfk_kind for non-canonical > > >> copy/move operations? That would presumably make it a tad more straightforward to go from > > >> fndecl to whatever class bits, instead of what's currently there, where we say "yeah I had a fndecl, > > >> now I turned it into an sfk_kind that says it's a copy constructor, but guess which one when you're > > >> deeming its triviality". ;) > > > > > > I suppose it would be possible to have a more detailed sfk_kind for > > > distinguishing between different signatures, but I'm inclined instead > > > to stop using sfk_kind in trivial_fn_p. Even if having an enumeration > > > is convenient for dispatch (or bitmapping), it doesn't need to be the > > > same enum. > > > > Yeah, the idea of using a different enum dawned on me straight after > > sending that email. ;) > > I'll give this approach a spin, more bits into the lang_type and a > > different mapping, that way we should indeed > > get correct answers for all cases. > > Hi Ville, any updates? No, and not any time soon. Do you by any chance want to pick this up? :)
On Wed, Dec 12, 2018 at 09:17:01AM +0200, Ville Voutilainen wrote: > On Tue, 11 Dec 2018 at 20:58, Marek Polacek <polacek@redhat.com> wrote: > > > > On Thu, Sep 13, 2018 at 08:58:34PM +0300, Ville Voutilainen wrote: > > > On 13 September 2018 at 20:41, Jason Merrill <jason@redhat.com> wrote: > > > >> Okay. Do you think we should have an sfk_kind for non-canonical > > > >> copy/move operations? That would presumably make it a tad more straightforward to go from > > > >> fndecl to whatever class bits, instead of what's currently there, where we say "yeah I had a fndecl, > > > >> now I turned it into an sfk_kind that says it's a copy constructor, but guess which one when you're > > > >> deeming its triviality". ;) > > > > > > > > I suppose it would be possible to have a more detailed sfk_kind for > > > > distinguishing between different signatures, but I'm inclined instead > > > > to stop using sfk_kind in trivial_fn_p. Even if having an enumeration > > > > is convenient for dispatch (or bitmapping), it doesn't need to be the > > > > same enum. > > > > > > Yeah, the idea of using a different enum dawned on me straight after > > > sending that email. ;) > > > I'll give this approach a spin, more bits into the lang_type and a > > > different mapping, that way we should indeed > > > get correct answers for all cases. > > > > Hi Ville, any updates? > > No, and not any time soon. Do you by any chance want to pick this up? :) Ok, this sounds interesting -- I'll give it a try. Marek
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c index 50b60e8..1b09721 100644 --- a/gcc/cp/decl.c +++ b/gcc/cp/decl.c @@ -13211,8 +13211,11 @@ grok_special_member_properties (tree decl) are no other parameters or else all other parameters have default arguments. */ TYPE_HAS_COPY_CTOR (class_type) = 1; - if (user_provided_p (decl)) - TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 1; + if (user_provided_p (decl) + && (ctor > 1 || !TYPE_HAS_CONST_COPY_CTOR (class_type))) + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 1; + else if (ctor > 1) + TYPE_HAS_COMPLEX_COPY_CTOR (class_type) = 0; if (ctor > 1) TYPE_HAS_CONST_COPY_CTOR (class_type) = 1; } diff --git a/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C b/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C new file mode 100644 index 0000000..177eb2e --- /dev/null +++ b/gcc/testsuite/g++.dg/ext/is_trivially_constructible7.C @@ -0,0 +1,16 @@ +// { dg-do compile { target c++11 } } + +// PR c++/87051 + +struct M { + M(const M&) = default; + M(M&); +}; + +struct M2 { + M2(M2&); + M2(const M2&) = default; +}; + +static_assert( __is_trivially_constructible(M, M&&), ""); +static_assert( __is_trivially_constructible(M2, M2&&), "");
Howdy! The tricky details of copy constructors, part 76. In this approach, I let the const-copy "dominate"; that is, if a const-copy was found, a non-const-copy will not turn off triviality of the copy constructor, and a const-copy will reinstate triviality of the copy constructor even if a non-const copy removed said triviality. I suppose we could try moving this knowledge from the class level to the level of special member function decl, but I wonder whether we have the bits to do that. This approach seems to solve the problem, certainly. This has been tested manually on Linux-x64, running full suite on Linux-PPC64. Thoughts? OK for trunk if the full suite passes? 2018-09-13 Ville Voutilainen <ville.voutilainen@gmail.com> gcc/cp PR c++/87051 * decl.c (grok_special_member_properties): Don't mark the class as having a non-trivial copy constructor if the copy constructor we're looking at is not a const-copy and a const-copy was already found, reset the copy constructor triviality if we found a trivial const-copy. testsuite/ PR c++/87051 * g++.dg/ext/is_trivially_constructible7.C: New.