diff mbox series

c++: Allow new char[4]{"foo"} [PR77841]

Message ID 20200901161032.4095949-1-polacek@redhat.com
State New
Headers show
Series c++: Allow new char[4]{"foo"} [PR77841] | expand

Commit Message

Marek Polacek Sept. 1, 2020, 4:10 p.m. UTC 
Currently, we allow new char[]{"foo"}, but not new char[4]{"foo"}.
We should accept the latter too: [dcl.init.list]p3.3 says to treat
this as [dcl.init.string].

We were rejecting this code because we never called reshape_init before
the digest_init in build_new_1.  reshape_init handles [dcl.init.string]
by unwrapping the STRING_CST from its enclosing { }, and digest_init
assumes that reshape_init has been called for aggregates anyway, and an
array is an aggregate.

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

gcc/cp/ChangeLog:

	PR c++/77841
	* init.c (build_new_1): Call reshape_init.

gcc/testsuite/ChangeLog:

	PR c++/77841
	* g++.dg/cpp0x/initlist-new4.C: New test.
---
 gcc/cp/init.c                              | 6 ++++++
 gcc/testsuite/g++.dg/cpp0x/initlist-new4.C | 6 ++++++
 2 files changed, 12 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-new4.C


base-commit: a292e31dac72c20cda3478b866ccf6e07dfad1a4

Comments

Jason Merrill Sept. 1, 2020, 7:27 p.m. UTC | #1 
On 9/1/20 12:10 PM, Marek Polacek wrote:
> Currently, we allow new char[]{"foo"}, but not new char[4]{"foo"}.
> We should accept the latter too: [dcl.init.list]p3.3 says to treat
> this as [dcl.init.string].
> 
> We were rejecting this code because we never called reshape_init before
> the digest_init in build_new_1.  reshape_init handles [dcl.init.string]
> by unwrapping the STRING_CST from its enclosing { }, and digest_init
> assumes that reshape_init has been called for aggregates anyway, and an
> array is an aggregate.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> gcc/cp/ChangeLog:
> 
> 	PR c++/77841
> 	* init.c (build_new_1): Call reshape_init.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR c++/77841
> 	* g++.dg/cpp0x/initlist-new4.C: New test.
> ---
>   gcc/cp/init.c                              | 6 ++++++
>   gcc/testsuite/g++.dg/cpp0x/initlist-new4.C | 6 ++++++
>   2 files changed, 12 insertions(+)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-new4.C
> 
> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
> index 360ab8c0b52..d4540db3605 100644
> --- a/gcc/cp/init.c
> +++ b/gcc/cp/init.c
> @@ -3575,6 +3575,12 @@ build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts,
>   		    /* We'll check the length at runtime.  */
>   		    domain = NULL_TREE;
>   		  arraytype = build_cplus_array_type (type, domain);
> +		  /* If we have new char[4]{"foo"}, we have to reshape
> +		     so that the STRING_CST isn't wrapped in { }.  */
> +		  vecinit = reshape_init (arraytype, vecinit, complain);
> +		  /* The middle end doesn't cope with the location wrapper
> +		     around a STRING_CST.  */
> +		  STRIP_ANY_LOCATION_WRAPPER (vecinit);
>   		  vecinit = digest_init (arraytype, vecinit, complain);
>   		}

This is OK, but now I wonder why your earlier addition,

>           /* 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);
>             }

isn't handled by the block you're changing in this patch.  Why isn't 
DIRECT_LIST_INIT_P true for new char[]{"foo"}?

Jason
Marek Polacek Sept. 1, 2020, 7:41 p.m. UTC | #2 
On Tue, Sep 01, 2020 at 03:27:36PM -0400, Jason Merrill via Gcc-patches wrote:
> On 9/1/20 12:10 PM, Marek Polacek wrote:
> > Currently, we allow new char[]{"foo"}, but not new char[4]{"foo"}.
> > We should accept the latter too: [dcl.init.list]p3.3 says to treat
> > this as [dcl.init.string].
> > 
> > We were rejecting this code because we never called reshape_init before
> > the digest_init in build_new_1.  reshape_init handles [dcl.init.string]
> > by unwrapping the STRING_CST from its enclosing { }, and digest_init
> > assumes that reshape_init has been called for aggregates anyway, and an
> > array is an aggregate.
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	PR c++/77841
> > 	* init.c (build_new_1): Call reshape_init.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	PR c++/77841
> > 	* g++.dg/cpp0x/initlist-new4.C: New test.
> > ---
> >   gcc/cp/init.c                              | 6 ++++++
> >   gcc/testsuite/g++.dg/cpp0x/initlist-new4.C | 6 ++++++
> >   2 files changed, 12 insertions(+)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-new4.C
> > 
> > diff --git a/gcc/cp/init.c b/gcc/cp/init.c
> > index 360ab8c0b52..d4540db3605 100644
> > --- a/gcc/cp/init.c
> > +++ b/gcc/cp/init.c
> > @@ -3575,6 +3575,12 @@ build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts,
> >   		    /* We'll check the length at runtime.  */
> >   		    domain = NULL_TREE;
> >   		  arraytype = build_cplus_array_type (type, domain);
> > +		  /* If we have new char[4]{"foo"}, we have to reshape
> > +		     so that the STRING_CST isn't wrapped in { }.  */
> > +		  vecinit = reshape_init (arraytype, vecinit, complain);
> > +		  /* The middle end doesn't cope with the location wrapper
> > +		     around a STRING_CST.  */
> > +		  STRIP_ANY_LOCATION_WRAPPER (vecinit);
> >   		  vecinit = digest_init (arraytype, vecinit, complain);
> >   		}
> 
> This is OK, but now I wonder why your earlier addition,
> 
> >           /* 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);
> >             }
> 
> isn't handled by the block you're changing in this patch.  Why isn't
> DIRECT_LIST_INIT_P true for new char[]{"foo"}?

Yes, I was hoping this hunk would handle the new char[4]{"foo"} case too,
but for new char[]{"foo"} DIRECT_LIST_INIT_P is false because earlier in
build_new we called reshape_init:

4011       /* Otherwise we should have 'new T[]{e_0, ..., e_k}'.  */
4012       if (BRACE_ENCLOSED_INITIALIZER_P (elt))
4013         elt = reshape_init (type, elt, complain);

which unwraps the { } from {"foo"}, so it's no longer a list init.  We
won't get there with new char[4]{"foo"} because TREE_CODE (type) will
not be ARRAY_TYPE; instead, nelts is set to INTEGER_CST 4 when we know
the array bound.

I could make it so that we call reshape_init in build_new for the [4]
case too, but it was uglier than this fix.

Should I go ahead with this patch as-is or would you prefer any changes?

Marek
Jason Merrill Sept. 1, 2020, 9:33 p.m. UTC | #3 
On 9/1/20 3:41 PM, Marek Polacek wrote:
> On Tue, Sep 01, 2020 at 03:27:36PM -0400, Jason Merrill via Gcc-patches wrote:
>> On 9/1/20 12:10 PM, Marek Polacek wrote:
>>> Currently, we allow new char[]{"foo"}, but not new char[4]{"foo"}.
>>> We should accept the latter too: [dcl.init.list]p3.3 says to treat
>>> this as [dcl.init.string].
>>>
>>> We were rejecting this code because we never called reshape_init before
>>> the digest_init in build_new_1.  reshape_init handles [dcl.init.string]
>>> by unwrapping the STRING_CST from its enclosing { }, and digest_init
>>> assumes that reshape_init has been called for aggregates anyway, and an
>>> array is an aggregate.
>>>
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	PR c++/77841
>>> 	* init.c (build_new_1): Call reshape_init.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	PR c++/77841
>>> 	* g++.dg/cpp0x/initlist-new4.C: New test.
>>> ---
>>>    gcc/cp/init.c                              | 6 ++++++
>>>    gcc/testsuite/g++.dg/cpp0x/initlist-new4.C | 6 ++++++
>>>    2 files changed, 12 insertions(+)
>>>    create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-new4.C
>>>
>>> diff --git a/gcc/cp/init.c b/gcc/cp/init.c
>>> index 360ab8c0b52..d4540db3605 100644
>>> --- a/gcc/cp/init.c
>>> +++ b/gcc/cp/init.c
>>> @@ -3575,6 +3575,12 @@ build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts,
>>>    		    /* We'll check the length at runtime.  */
>>>    		    domain = NULL_TREE;
>>>    		  arraytype = build_cplus_array_type (type, domain);
>>> +		  /* If we have new char[4]{"foo"}, we have to reshape
>>> +		     so that the STRING_CST isn't wrapped in { }.  */
>>> +		  vecinit = reshape_init (arraytype, vecinit, complain);
>>> +		  /* The middle end doesn't cope with the location wrapper
>>> +		     around a STRING_CST.  */
>>> +		  STRIP_ANY_LOCATION_WRAPPER (vecinit);
>>>    		  vecinit = digest_init (arraytype, vecinit, complain);
>>>    		}
>>
>> This is OK, but now I wonder why your earlier addition,
>>
>>>            /* 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);
>>>              }
>>
>> isn't handled by the block you're changing in this patch.  Why isn't
>> DIRECT_LIST_INIT_P true for new char[]{"foo"}?
> 
> Yes, I was hoping this hunk would handle the new char[4]{"foo"} case too,
> but for new char[]{"foo"} DIRECT_LIST_INIT_P is false because earlier in
> build_new we called reshape_init:
> 
> 4011       /* Otherwise we should have 'new T[]{e_0, ..., e_k}'.  */
> 4012       if (BRACE_ENCLOSED_INITIALIZER_P (elt))
> 4013         elt = reshape_init (type, elt, complain);
> 
> which unwraps the { } from {"foo"}, so it's no longer a list init.

Ah, I see.

> We
> won't get there with new char[4]{"foo"} because TREE_CODE (type) will
> not be ARRAY_TYPE; instead, nelts is set to INTEGER_CST 4 when we know
> the array bound.
> 
> I could make it so that we call reshape_init in build_new for the [4]
> case too, but it was uglier than this fix.
> 
> Should I go ahead with this patch as-is or would you prefer any changes?

Go ahead with this for now, but I notice that we also still don't support

new char[4](1,2,3,4);

because the handling of parenthesized-init is limited to the deduced 
array size case.

It would be nice to find a way to combine the two places that we're 
messing with array initializers.

Jason
Marek Polacek Sept. 1, 2020, 10:38 p.m. UTC | #4 
On Tue, Sep 01, 2020 at 05:33:43PM -0400, Jason Merrill via Gcc-patches wrote:
> On 9/1/20 3:41 PM, Marek Polacek wrote:
> > On Tue, Sep 01, 2020 at 03:27:36PM -0400, Jason Merrill via Gcc-patches wrote:
> > > On 9/1/20 12:10 PM, Marek Polacek wrote:
> > > > Currently, we allow new char[]{"foo"}, but not new char[4]{"foo"}.
> > > > We should accept the latter too: [dcl.init.list]p3.3 says to treat
> > > > this as [dcl.init.string].
> > > > 
> > > > We were rejecting this code because we never called reshape_init before
> > > > the digest_init in build_new_1.  reshape_init handles [dcl.init.string]
> > > > by unwrapping the STRING_CST from its enclosing { }, and digest_init
> > > > assumes that reshape_init has been called for aggregates anyway, and an
> > > > array is an aggregate.
> > > > 
> > > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > > > 
> > > > gcc/cp/ChangeLog:
> > > > 
> > > > 	PR c++/77841
> > > > 	* init.c (build_new_1): Call reshape_init.
> > > > 
> > > > gcc/testsuite/ChangeLog:
> > > > 
> > > > 	PR c++/77841
> > > > 	* g++.dg/cpp0x/initlist-new4.C: New test.
> > > > ---
> > > >    gcc/cp/init.c                              | 6 ++++++
> > > >    gcc/testsuite/g++.dg/cpp0x/initlist-new4.C | 6 ++++++
> > > >    2 files changed, 12 insertions(+)
> > > >    create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-new4.C
> > > > 
> > > > diff --git a/gcc/cp/init.c b/gcc/cp/init.c
> > > > index 360ab8c0b52..d4540db3605 100644
> > > > --- a/gcc/cp/init.c
> > > > +++ b/gcc/cp/init.c
> > > > @@ -3575,6 +3575,12 @@ build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts,
> > > >    		    /* We'll check the length at runtime.  */
> > > >    		    domain = NULL_TREE;
> > > >    		  arraytype = build_cplus_array_type (type, domain);
> > > > +		  /* If we have new char[4]{"foo"}, we have to reshape
> > > > +		     so that the STRING_CST isn't wrapped in { }.  */
> > > > +		  vecinit = reshape_init (arraytype, vecinit, complain);
> > > > +		  /* The middle end doesn't cope with the location wrapper
> > > > +		     around a STRING_CST.  */
> > > > +		  STRIP_ANY_LOCATION_WRAPPER (vecinit);
> > > >    		  vecinit = digest_init (arraytype, vecinit, complain);
> > > >    		}
> > > 
> > > This is OK, but now I wonder why your earlier addition,
> > > 
> > > >            /* 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);
> > > >              }
> > > 
> > > isn't handled by the block you're changing in this patch.  Why isn't
> > > DIRECT_LIST_INIT_P true for new char[]{"foo"}?
> > 
> > Yes, I was hoping this hunk would handle the new char[4]{"foo"} case too,
> > but for new char[]{"foo"} DIRECT_LIST_INIT_P is false because earlier in
> > build_new we called reshape_init:
> > 
> > 4011       /* Otherwise we should have 'new T[]{e_0, ..., e_k}'.  */
> > 4012       if (BRACE_ENCLOSED_INITIALIZER_P (elt))
> > 4013         elt = reshape_init (type, elt, complain);
> > 
> > which unwraps the { } from {"foo"}, so it's no longer a list init.
> 
> Ah, I see.
> 
> > We
> > won't get there with new char[4]{"foo"} because TREE_CODE (type) will
> > not be ARRAY_TYPE; instead, nelts is set to INTEGER_CST 4 when we know
> > the array bound.
> > 
> > I could make it so that we call reshape_init in build_new for the [4]
> > case too, but it was uglier than this fix.
> > 
> > Should I go ahead with this patch as-is or would you prefer any changes?
> 
> Go ahead with this for now, but I notice that we also still don't support

Pushed.

> new char[4](1,2,3,4);
> 
> because the handling of parenthesized-init is limited to the deduced array
> size case.

Ah, that should work.  And conversely here I'd expect an error:

new char[2]("so_sad");

but we don't give any.

> It would be nice to find a way to combine the two places that we're messing
> with array initializers.

I'll look into that.  Thanks,

Marek
diff mbox series

Patch

diff --git a/gcc/cp/init.c b/gcc/cp/init.c
index 360ab8c0b52..d4540db3605 100644
--- a/gcc/cp/init.c
+++ b/gcc/cp/init.c
@@ -3575,6 +3575,12 @@  build_new_1 (vec<tree, va_gc> **placement, tree type, tree nelts,
 		    /* We'll check the length at runtime.  */
 		    domain = NULL_TREE;
 		  arraytype = build_cplus_array_type (type, domain);
+		  /* If we have new char[4]{"foo"}, we have to reshape
+		     so that the STRING_CST isn't wrapped in { }.  */
+		  vecinit = reshape_init (arraytype, vecinit, complain);
+		  /* The middle end doesn't cope with the location wrapper
+		     around a STRING_CST.  */
+		  STRIP_ANY_LOCATION_WRAPPER (vecinit);
 		  vecinit = digest_init (arraytype, vecinit, complain);
 		}
 	    }
diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-new4.C b/gcc/testsuite/g++.dg/cpp0x/initlist-new4.C
new file mode 100644
index 00000000000..d7b418426bb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/initlist-new4.C
@@ -0,0 +1,6 @@ 
+// PR c++/77841
+// { dg-do compile { target c++11 } }
+
+char *p1 = new char[4]{"foo"};
+char *p2 = new char[5]{"foo"};
+char *p3 = new char[3]{"foo"}; // { dg-error "too long" }