Re: [PATCH v2] c++: direct-init of an array of class type [PR59465]

[Jason Merrill <jason@redhat.com>]

On 3/21/24 16:48, Marek Polacek wrote:
> On Wed, Mar 20, 2024 at 09:21:02PM -0400, Jason Merrill wrote:
>> On 3/1/24 19:58, Marek Polacek wrote:
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?  I don't
>>> claim that this has to go to 14 though.
>>>
>>> -- >8 --
>>> ...from another array in a mem-initializer should not be accepted.
>>>
>>> We already reject
>>>
>>>     struct string {} a[1];
>>>     string x[1](a);
>>>
>>> but
>>>
>>>     struct pair {
>>>       string s[1];
>>>       pair() : s(a) {}
>>>     };
>>>
>>> is wrongly accepted.
>>>
>>> It started to be accepted with r0-110915-ga034826198b771:
>>> <https://gcc.gnu.org/pipermail/gcc-patches/2011-August/320236.html>
>>> which was supposed to be a cleanup, not a deliberate change to start
>>> accepting the code.  The build_vec_init_expr code was added in r165976:
>>> <https://gcc.gnu.org/pipermail/gcc-patches/2010-October/297582.html>.
>>>
>>> It appears that we do the magic copy array when we have a defaulted
>>> constructor and we generate code for its mem-initializer which
>>> initializes an array.  I also see that we go that path for compound
>>> literals.  So when initializing an array member, we can limit building
>>> up a VEC_INIT_EXPR to those special cases.
>>>
>>> 	PR c++/59465
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* init.cc (can_init_array_with_p): New.
>>> 	(perform_member_init): Check it.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/init/array62.C: New test.
>>> 	* g++.dg/init/array63.C: New test.
>>> 	* g++.dg/init/array64.C: New test.
>>> ---
>>>    gcc/cp/init.cc                      | 27 ++++++++++++++++++++++++++-
>>>    gcc/testsuite/g++.dg/init/array62.C | 19 +++++++++++++++++++
>>>    gcc/testsuite/g++.dg/init/array63.C | 13 +++++++++++++
>>>    gcc/testsuite/g++.dg/init/array64.C | 22 ++++++++++++++++++++++
>>>    4 files changed, 80 insertions(+), 1 deletion(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/init/array62.C
>>>    create mode 100644 gcc/testsuite/g++.dg/init/array63.C
>>>    create mode 100644 gcc/testsuite/g++.dg/init/array64.C
>>>
>>> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
>>> index d2586fad86b..fb8c0e521fb 100644
>>> --- a/gcc/cp/init.cc
>>> +++ b/gcc/cp/init.cc
>>> @@ -934,6 +934,31 @@ find_uninit_fields (tree *t, hash_set<tree> *uninitialized, tree member)
>>>        }
>>>    }
>>> +/* Return true if it's OK to initialize an array from INIT.  Mere mortals
>>> +   can't copy arrays, but the compiler can do so with a VEC_INIT_EXPR in
>>> +   certain cases.  */
>>> +
>>> +static bool
>>> +can_init_array_with_p (tree init)
>>> +{
>>> +  if (!init)
>>> +    return true;
>>> +
>>> +  /* We're called from synthesize_method, and we're processing the
>>> +     mem-initializers of a constructor.  */
>>> +  if (DECL_DEFAULTED_FN (current_function_decl))
>>> +    return true;
>>> +  /* As an extension, we allow copying from a compound literal.  */
>>> +  else if (TREE_CODE (init) == TARGET_EXPR)
>>> +    {
>>> +      init = TARGET_EXPR_INITIAL (init);
>>> +      if (TREE_CODE (init) == CONSTRUCTOR)
>>> +	return CONSTRUCTOR_C99_COMPOUND_LITERAL (init);
>>> +    }
>>> +
>>> +  return false;
>>> +}
>>> +
>>>    /* Initialize MEMBER, a FIELD_DECL, with INIT, a TREE_LIST of
>>>       arguments.  If TREE_LIST is void_type_node, an empty initializer
>>>       list was given; if NULL_TREE no initializer was given.  UNINITIALIZED
>>> @@ -1085,7 +1110,7 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized)
>>>      else if (type_build_ctor_call (type)
>>>    	   || (init && CLASS_TYPE_P (strip_array_types (type))))
>>>        {
>>> -      if (TREE_CODE (type) == ARRAY_TYPE)
>>> +      if (TREE_CODE (type) == ARRAY_TYPE && can_init_array_with_p (init))
>>>    	{
>>>    	  if (init == NULL_TREE
>>>    	      || same_type_ignoring_top_level_qualifiers_p (type,
>>
>> It seems like these last two existing lines also fall under "init is
>> suitable to initialize type", so let's fold them into the new function.
> 
> Sounds good.  Here it is:
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

OK.

> -- >8 --
> ...from another array in a mem-initializer should not be accepted.
> 
> We already reject
> 
>    struct string {} a[1];
>    string x[1](a);
> 
> but
> 
>    struct pair {
>      string s[1];
>      pair() : s(a) {}
>    };
> 
> is wrongly accepted.
> 
> It started to be accepted with r0-110915-ga034826198b771:
> <https://gcc.gnu.org/pipermail/gcc-patches/2011-August/320236.html>
> which was supposed to be a cleanup, not a deliberate change to start
> accepting the code.  The build_vec_init_expr code was added in r165976:
> <https://gcc.gnu.org/pipermail/gcc-patches/2010-October/297582.html>.
> 
> It appears that we do the magic copy array when we have a defaulted
> constructor and we generate code for its mem-initializer which
> initializes an array.  I also see that we go that path for compound
> literals.  So when initializing an array member, we can limit building
> up a VEC_INIT_EXPR to those special cases.
> 
> 	PR c++/59465
> 
> gcc/cp/ChangeLog:
> 
> 	* init.cc (can_init_array_with_p): New.
> 	(perform_member_init): Check it.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/init/array62.C: New test.
> 	* g++.dg/init/array63.C: New test.
> 	* g++.dg/init/array64.C: New test.
> ---
>   gcc/cp/init.cc                      | 31 ++++++++++++++++++++++++++---
>   gcc/testsuite/g++.dg/init/array62.C | 19 ++++++++++++++++++
>   gcc/testsuite/g++.dg/init/array63.C | 13 ++++++++++++
>   gcc/testsuite/g++.dg/init/array64.C | 22 ++++++++++++++++++++
>   4 files changed, 82 insertions(+), 3 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/init/array62.C
>   create mode 100644 gcc/testsuite/g++.dg/init/array63.C
>   create mode 100644 gcc/testsuite/g++.dg/init/array64.C
> 
> diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc
> index d2586fad86b..dbd37d47cbf 100644
> --- a/gcc/cp/init.cc
> +++ b/gcc/cp/init.cc
> @@ -934,6 +934,33 @@ find_uninit_fields (tree *t, hash_set<tree> *uninitialized, tree member)
>       }
>   }
>   
> +/* Return true if it's OK to initialize an array TYPE from INIT.  Mere mortals
> +   can't copy arrays, but the compiler can do so with a VEC_INIT_EXPR in
> +   certain cases.  */
> +
> +static bool
> +can_init_array_with_p (tree type, tree init)
> +{
> +  if (!init)
> +    /* Value-init, OK.  */
> +    return true;
> +  if (!same_type_ignoring_top_level_qualifiers_p (type, TREE_TYPE (init)))
> +    return false;
> +  /* We're called from synthesize_method, and we're processing the
> +     mem-initializers of a constructor.  */
> +  if (DECL_DEFAULTED_FN (current_function_decl))
> +    return true;
> +  /* As an extension, we allow copying from a compound literal.  */
> +  if (TREE_CODE (init) == TARGET_EXPR)
> +    {
> +      init = TARGET_EXPR_INITIAL (init);
> +      if (TREE_CODE (init) == CONSTRUCTOR)
> +	return CONSTRUCTOR_C99_COMPOUND_LITERAL (init);
> +    }
> +
> +  return false;
> +}
> +
>   /* Initialize MEMBER, a FIELD_DECL, with INIT, a TREE_LIST of
>      arguments.  If TREE_LIST is void_type_node, an empty initializer
>      list was given; if NULL_TREE no initializer was given.  UNINITIALIZED
> @@ -1087,9 +1114,7 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized)
>       {
>         if (TREE_CODE (type) == ARRAY_TYPE)
>   	{
> -	  if (init == NULL_TREE
> -	      || same_type_ignoring_top_level_qualifiers_p (type,
> -							    TREE_TYPE (init)))
> +	  if (can_init_array_with_p (type, init))
>   	    {
>   	      if (TYPE_DOMAIN (type) && TYPE_MAX_VALUE (TYPE_DOMAIN (type)))
>   		{
> diff --git a/gcc/testsuite/g++.dg/init/array62.C b/gcc/testsuite/g++.dg/init/array62.C
> new file mode 100644
> index 00000000000..2a786a36e4e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/init/array62.C
> @@ -0,0 +1,19 @@
> +// PR c++/59465
> +// { dg-do compile }
> +
> +struct string {} a[1];
> +struct pair {
> +  string s[1];
> +  pair() : s(a) {} // { dg-error "invalid initializer for array member" }
> +};
> +
> +struct S {
> +  char s[10];
> +  S() : s("aaa") {}
> +};
> +
> +void
> +g ()
> +{
> +  string x[1](a); // { dg-error "array must be initialized" }
> +}
> diff --git a/gcc/testsuite/g++.dg/init/array63.C b/gcc/testsuite/g++.dg/init/array63.C
> new file mode 100644
> index 00000000000..57e98056168
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/init/array63.C
> @@ -0,0 +1,13 @@
> +// PR c++/59465
> +// { dg-do compile }
> +
> +struct I {
> +    const bool b;
> +};
> +struct O {
> +    I a[2];
> +    static I const data[2];
> +    O() : a(data){}  // { dg-error "invalid initializer for array member" }
> +};
> +
> +I const O::data[2] = {true, false};
> diff --git a/gcc/testsuite/g++.dg/init/array64.C b/gcc/testsuite/g++.dg/init/array64.C
> new file mode 100644
> index 00000000000..e0afdfab39a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/init/array64.C
> @@ -0,0 +1,22 @@
> +// PR c++/59465
> +// { dg-do compile }
> +
> +static const int my_size = 10;
> +
> +class UserType
> +{
> +public:
> +  UserType(): f_(){}
> +private:
> +int f_;
> +};
> +
> +typedef UserType Array[my_size];
> +
> +class Foo
> +{
> +public:
> +  Foo(Array& m) : m_(m) {};  // { dg-error "invalid initializer for array member" }
> +private:
> +  Array m_;
> +};
> 
> base-commit: 509352069d6f166d396f4b4a86e71ea521f2ca78