On Wed, Sep 09, 2020 at 05:02:24PM -0400, Jason Merrill wrote:
> On 9/8/20 10:34 PM, Marek Polacek wrote:
> > On Tue, Sep 08, 2020 at 04:19:42PM -0400, Jason Merrill wrote:
> > > On 9/8/20 4:06 PM, Marek Polacek wrote:
> > > > On Mon, Sep 07, 2020 at 11:19:47PM -0400, Jason Merrill wrote:
> > > > > On 9/6/20 11:34 AM, Marek Polacek wrote:
> > > > > > @@ -3944,9 +3935,9 @@ build_new (location_t loc, vec<tree, va_gc>
> > > > > > **placement, tree type,
> > > > > > }
> > > > > > /* P1009: Array size deduction in new-expressions. */
> > > > > > - if (TREE_CODE (type) == ARRAY_TYPE
> > > > > > - && !TYPE_DOMAIN (type)
> > > > > > - && *init)
> > > > > > + const bool deduce_array_p = (TREE_CODE (type) == ARRAY_TYPE
> > > > > > + && !TYPE_DOMAIN (type));
> > > > > > + if (*init && (deduce_array_p || (nelts && cxx_dialect >= cxx20)))
> > > > >
> > > > > Looks like this won't handle new (char[4]), for which we also get an
> > > > > ARRAY_TYPE.
> > > >
> > > > Good catch. Fixed & paren-init37.C added.
> > > >
> > > > > > {
> > > > > > /* This means we have 'new T[]()'. */
> > > > > > if ((*init)->is_empty ())
> > > > > > @@ -3955,16 +3946,20 @@ build_new (location_t loc, vec<tree, va_gc>
> > > > > > **placement, tree type,
> > > > > > CONSTRUCTOR_IS_DIRECT_INIT (ctor) = true;
> > > > > > vec_safe_push (*init, ctor);
> > > > > > }
> > > > > > + tree array_type = deduce_array_p ? TREE_TYPE (type) : type;
> > > > >
> > > > > I'd call this variable elt_type.
> > > >
> > > > Right, and it should be inside the block below.
> > > >
> > > > > > tree &elt = (**init)[0];
> > > > > > /* The C++20 'new T[](e_0, ..., e_k)' case allowed by
> > > > > > P0960. */
> > > > > > if (!DIRECT_LIST_INIT_P (elt) && cxx_dialect >= cxx20)
> > > > > > {
> > > > > > - /* Handle new char[]("foo"). */
> > > > > > + /* Handle new char[]("foo"): turn it into new char[]{"foo"}.
> > > > > > */
> > > > > > if (vec_safe_length (*init) == 1
> > > > > > - && char_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (type)))
> > > > > > + && char_type_p (TYPE_MAIN_VARIANT (array_type))
> > > > > > && TREE_CODE (tree_strip_any_location_wrapper
> > > > > > (elt))
> > > > > > == STRING_CST)
> > > > > > - /* Leave it alone: the string should not be wrapped in {}.
> > > > > > */;
> > > > > > + {
> > > > > > + elt = build_constructor_single (init_list_type_node,
> > > > > > NULL_TREE, elt);
> > > > > > + CONSTRUCTOR_IS_DIRECT_INIT (elt) = true;
> > > > > > + }
> > > > > > else
> > > > > > {
> > > > > > tree ctor = build_constructor_from_vec
> > > > > > (init_list_type_node, *init);
> > > > >
> > > > > With this change, doesn't the string special case produce the same
> > > > > result as
> > > > > the general case?
> > > >
> > > > The problem is that reshape_init won't do anything for
> > > > CONSTRUCTOR_IS_PAREN_INIT.
> > >
> > > Ah, yes, that flag is the difference.
> > >
> > > > So the reshape_init in build_new_1 wouldn't unwrap the outermost { }
> > > > around
> > > > a STRING_CST.
> > >
> > > > Perhaps reshape_init should be adjusted to do that unwrapping even when
> > > > it gets
> > > > a CONSTRUCTOR_IS_PAREN_INIT CONSTRUCTOR. But I'm not sure if it should
> > > > also do
> > > > the reference_related_p unwrapping in reshape_init_r in that case.
> > >
> > > That would make sense to me.
> >
> > Done (but only for the outermost CONSTRUCTOR) in the below. It allowed me
> > to...
> >
> > > > > > @@ -3977,9 +3972,15 @@ build_new (location_t loc, vec<tree, va_gc>
> > > > > > **placement, tree type,
> > > > > > }
> > > > > > }
> > > > > > /* Otherwise we should have 'new T[]{e_0, ..., e_k}'. */
> > > > > > - if (BRACE_ENCLOSED_INITIALIZER_P (elt))
> > > > > > - elt = reshape_init (type, elt, complain);
> > > > > > - cp_complete_array_type (&type, elt, /*do_default*/false);
> > > > > > + if (deduce_array_p)
> > > > > > + {
> > > > > > + /* Don't reshape ELT itself: we want to pass a
> > > > > > list-initializer to
> > > > > > + build_new_1, even for STRING_CSTs. */
> > > > > > + tree e = elt;
> > > > > > + if (BRACE_ENCLOSED_INITIALIZER_P (e))
> > > > > > + e = reshape_init (type, e, complain);
> > > > >
> > > > > The comment is unclear; this call does reshape the CONSTRUCTOR ELT
> > > > > points
> > > > > to, it just doesn't change ELT if the reshape call returns something
> > > > > else.
> > > >
> > > > Yea, I've amended the comment.
> > > >
> > > > > Why are we reshaping here, anyway? Won't that lead to undesired brace
> > > > > elision?
> > > >
> > > > We have to reshape before deducing the array, otherwise we could deduce
> > > > the
> > > > wrong number of elements when certain braces were omitted. E.g. in
> > > >
> > > > struct S { int x, y; };
> > > > new S[]{1, 2, 3, 4}; // braces elided, is { {1, 2}, {3, 4} }
> > >
> > > Ah, right, we also get here for initializers written with actual braces.
> > >
> > > > we want S[2], not S[4]. A way to test it would be
> > > >
> > > > struct S { int x, y; };
> > > > S *p = new S[]{1, 2, 3, 4};
> > > >
> > > > void* operator new (unsigned long int size)
> > > > {
> > > > if (size != sizeof (S) * 2)
> > > > __builtin_abort ();
> > > > return __builtin_malloc (size);
> > > > }
> > > >
> > > > int main () { }
> > > >
> > > > I can add that too, if you want. (It'd be safer if
> > > > cp_complete_array_type
> > > > always reshaped but that's not trivial, as the original patch mentions.)
> > > > ()-init-list wouldn't be reshaped because CONSTRUCTOR_IS_PAREN_INIT is
> > > > set.
> > > >
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > >
> > > > Thanks,
> > > >
> > > > -- >8 --
> > > > This patch corrects our handling of array new-expression with ()-init:
> > > >
> > > > new int[4](1, 2, 3, 4);
> > > >
> > > > should work even with the explicit array bound, and
> > > >
> > > > new char[3]("so_sad");
> > > >
> > > > should cause an error, but we weren't giving any.
> > > >
> > > > Fixed by handling array new-expressions with ()-init in the same spot
> > > > where we deduce the array bound in array new-expression. I'm now
> > > > always passing STRING_CSTs to build_new_1 wrapped in { } which allowed
> > > > me to remove the special handling of STRING_CSTs in build_new_1. And
> > > > since the DIRECT_LIST_INIT_P block in build_new_1 calls digest_init, we
> > > > report errors about too short arrays.
> > > >
> > > > I took a stab at cp_complete_array_type's "FIXME: this code is
> > > > duplicated
> > > > from reshape_init", but calling reshape_init there, I ran into issues
> > > > with has_designator_problem: when we reshape an already reshaped
> > > > CONSTRUCTOR again, d.cur.index has been filled, so we think that we
> > > > have a user-provided designator (though there was no designator in the
> > > > source code), and report an error.
> > > >
> > > > gcc/cp/ChangeLog:
> > > >
> > > > PR c++/77841
> > > > * init.c (build_new_1): Don't handle string-initializers here.
> > > > (build_new): Handle new-expression with paren-init when the
> > > > array bound is known. Always pass string constants to
> > > > build_new_1
> > > > enclosed in braces.
> > > >
> > > > gcc/testsuite/ChangeLog:
> > > >
> > > > PR c++/77841
> > > > * g++.old-deja/g++.ext/arrnew2.C: Expect the error only in C++17
> > > > and less.
> > > > * g++.old-deja/g++.robertl/eb58.C: Adjust dg-error.
> > > > * g++.old-deja/g++.robertl/eb63.C: Expect the error only in
> > > > C++17
> > > > and less.
> > > > * g++.dg/cpp2a/paren-init36.C: New test.
> > > > * g++.dg/cpp2a/paren-init37.C: New test.
> > > > * g++.dg/pr84729.C: Adjust dg-error.
> > > > ---
> > > > gcc/cp/init.c | 41
> > > > +++++++++++--------
> > > > gcc/testsuite/g++.dg/cpp2a/paren-init36.C | 14 +++++++
> > > > gcc/testsuite/g++.dg/cpp2a/paren-init37.C | 14 +++++++
> > > > gcc/testsuite/g++.dg/pr84729.C | 2 +-
> > > > gcc/testsuite/g++.old-deja/g++.ext/arrnew2.C | 2 +-
> > > > gcc/testsuite/g++.old-deja/g++.robertl/eb58.C | 2 +-
> > > > gcc/testsuite/g++.old-deja/g++.robertl/eb63.C | 2 +-
> > > > 7 files changed, 55 insertions(+), 22 deletions(-)
> > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/paren-init36.C
> > > > create mode 100644 gcc/testsuite/g++.dg/cpp2a/paren-init37.C
> > > >
> > > > diff --git a/gcc/cp/init.c b/gcc/cp/init.c
> > > > index 3268ae4ad3f..fe4d49df402 100644
> > > > --- a/gcc/cp/init.c
> > > > +++ b/gcc/cp/init.c
> > > > @@ -3596,15 +3596,6 @@ build_new_1 (vec<tree, va_gc> **placement, tree
> > > > type, tree nelts,
> > > > vecinit = digest_init (arraytype, vecinit, complain);
> > > > }
> > > > }
> > > > - /* This handles code like new char[]{"foo"}. */
> > > > - else if (len == 1
> > > > - && char_type_p (TYPE_MAIN_VARIANT (type))
> > > > - && TREE_CODE (tree_strip_any_location_wrapper
> > > > ((**init)[0]))
> > > > - == STRING_CST)
> > > > - {
> > > > - vecinit = (**init)[0];
> > > > - STRIP_ANY_LOCATION_WRAPPER (vecinit);
> > > > - }
> > > > else if (*init)
> > > > {
> > > > if (complain & tf_error)
> > > > @@ -3944,9 +3935,8 @@ build_new (location_t loc, vec<tree, va_gc>
> > > > **placement, tree type,
> > > > }
> > > > /* P1009: Array size deduction in new-expressions. */
> > > > - if (TREE_CODE (type) == ARRAY_TYPE
> > > > - && !TYPE_DOMAIN (type)
> > > > - && *init)
> > > > + const bool array_p = TREE_CODE (type) == ARRAY_TYPE;
> > > > + if (*init && (array_p || (nelts && cxx_dialect >= cxx20)))
> > > > {
> > > > /* This means we have 'new T[]()'. */
> > > > if ((*init)->is_empty ())
> > > > @@ -3959,12 +3949,16 @@ build_new (location_t loc, vec<tree, va_gc>
> > > > **placement, tree type,
> > > > /* The C++20 'new T[](e_0, ..., e_k)' case allowed by P0960.
> > > > */
> > > > if (!DIRECT_LIST_INIT_P (elt) && cxx_dialect >= cxx20)
> > > > {
> > > > - /* Handle new char[]("foo"). */
> > > > + tree elt_type = array_p ? TREE_TYPE (type) : type;
> > >
> > > I think this should condition on whether nelts is set, to handle e.g. new
> > > char[2][2] properly.
> >
> > ...remove this code.
> >
> > I've added new-array5.C to test multidimensional arrays too.
> >
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> >
> > -- >8 --
> > This patch corrects our handling of array new-expression with ()-init:
> >
> > new int[4](1, 2, 3, 4);
> >
> > should work even with the explicit array bound, and
> >
> > new char[3]("so_sad");
> >
> > should cause an error, but we weren't giving any.
> >
> > Fixed by handling array new-expressions with ()-init in the same spot
> > where we deduce the array bound in array new-expression. I'm now
> > always passing STRING_CSTs to build_new_1 wrapped in { } which allowed
> > me to remove the special handling of STRING_CSTs in build_new_1. And
> > since the DIRECT_LIST_INIT_P block in build_new_1 calls digest_init, we
> > report errors about too short arrays. reshape_init now does the {"foo"}
> > -> "foo" transformation even for CONSTRUCTOR_IS_PAREN_INIT, so no need
> > to do it in build_new.
> >
> > I took a stab at cp_complete_array_type's "FIXME: this code is duplicated
> > from reshape_init", but calling reshape_init there, I ran into issues
> > with has_designator_problem: when we reshape an already reshaped
> > CONSTRUCTOR again, d.cur.index has been filled, so we think that we
> > have a user-provided designator (though there was no designator in the
> > source code), and report an error.
> >
> > gcc/cp/ChangeLog:
> >
> > PR c++/77841
> > * decl.c (reshape_init): If we're initializing a char array from
> > a string-literal that is enclosed in braces, unwrap it.
> > * init.c (build_new_1): Don't handle string-initializers here.
> > (build_new): Handle new-expression with paren-init when the
> > array bound is known. Always pass string constants to build_new_1
> > enclosed in braces. Don't handle string-initializers in any
> > special way.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR c++/77841
> > * g++.old-deja/g++.ext/arrnew2.C: Expect the error only in C++17
> > and less.
> > * g++.old-deja/g++.robertl/eb58.C: Adjust dg-error.
> > * g++.old-deja/g++.robertl/eb63.C: Expect the error only in C++17
> > and less.
> > * g++.dg/cpp2a/new-array5.C: New test.
> > * g++.dg/cpp2a/paren-init36.C: New test.
> > * g++.dg/cpp2a/paren-init37.C: New test.
> > * g++.dg/pr84729.C: Adjust dg-error.
> > ---
> > gcc/cp/decl.c | 12 ++++-
> > gcc/cp/init.c | 54 ++++++++-----------
> > gcc/testsuite/g++.dg/cpp2a/new-array5.C | 15 ++++++
> > gcc/testsuite/g++.dg/cpp2a/paren-init36.C | 14 +++++
> > gcc/testsuite/g++.dg/cpp2a/paren-init37.C | 14 +++++
> > gcc/testsuite/g++.dg/pr84729.C | 2 +-
> > gcc/testsuite/g++.old-deja/g++.ext/arrnew2.C | 2 +-
> > gcc/testsuite/g++.old-deja/g++.robertl/eb58.C | 2 +-
> > gcc/testsuite/g++.old-deja/g++.robertl/eb63.C | 2 +-
> > 9 files changed, 81 insertions(+), 36 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/cpp2a/new-array5.C
> > create mode 100644 gcc/testsuite/g++.dg/cpp2a/paren-init36.C
> > create mode 100644 gcc/testsuite/g++.dg/cpp2a/paren-init37.C
> >
> > diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> > index 31d68745844..1287ce1efd1 100644
> > --- a/gcc/cp/decl.c
> > +++ b/gcc/cp/decl.c
> > @@ -6599,7 +6599,17 @@ reshape_init (tree type, tree init, tsubst_flags_t
> > complain)
> > /* Brace elision is not performed for a CONSTRUCTOR representing
> > parenthesized aggregate initialization. */
> > if (CONSTRUCTOR_IS_PAREN_INIT (init))
> > - return init;
> > + {
> > + tree elt = (*v)[0].value;
> > + /* If we're initializing a char array from a string-literal that is
> > + enclosed in braces, unwrap it here. */
> > + if (TREE_CODE (type) == ARRAY_TYPE
> > + && vec_safe_length (v) == 1
> > + && char_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (type)))
> > + && TREE_CODE (tree_strip_any_location_wrapper (elt)) == STRING_CST)
> > + return elt;
> > + return init;
>
> You decided against unwrapping a reference_related_p initializer here?
Yeah, it doesn't seem to be needed: e.g. in
struct X { int i; };
struct Y : X { };
int main ()
{
Y y;
y.i = 42;
X x2(y);
__builtin_printf ("%d\n", x2.i);
}
we don't reshape_init at all, and build_aggr_init creates an INIT_EXPR
x2 = y.D.2087
which is the same code we generate with e.g. GCC 8, or if we use x2{y}.
Also, P0960 says that any existing meaning of A(b) should not change.
Marek
