[PATCH] c++: designated init of char array by string constant [PR55227]

[Marek Polacek]

Hi,

thanks for the patch and sorry for the delay in reviewing.

On Sat, Nov 06, 2021 at 08:17:23PM -0400, Will Wray via Gcc-patches wrote:
> This patch aims to fix https://gcc.gnu.org/bugzilla/show_bug.cgi?id=55227.
> 
> There are two underlying bugs in the designated initialization of char array
> fields by string literals that cause:
> 
> (1) Rejection of valid cases with:
>   (a) brace-enclosed string literal initializer (of any valid size), or
>   (b) unbraced string literal shorter than the target char array field.
> 
> (2) Acceptance of invalid cases with designators appearing within the braces
>     of a braced string literal, in which case the bogus 'designator' was being
>     entirely ignored and the string literal treated as a positional initializer.

I also noticed the C++ FE rejects

  struct A { char x[4]; };
  struct B { struct A a; };
  struct B b = { .a.x = "abc" };
 
but the C FE accepts it.  But that's for another time.

> Please review these changes carefully; there are likely errors of omission,
> logic or an anon anomaly.
> 
> The fixes above allow to address a FIXME in cp_complete_array_type:
> 
>   /* FIXME: this code is duplicated from reshape_init.
>      Probably we should just call reshape_init here?  */
> 
> I believe that this was obstructed by the designator bugs (see comment here
> https://patchwork.ozlabs.org/project/gcc/list/?series=199783)
> 
> Boostraps/regtests on x86_64-pc-linux-gnu.
> 
>         PR c++/55227
> 
> gcc/cp/ChangeLog:
> 
>         * decl.c (reshape_init_r): restrict has_designator_check,
  
I think something like "Only call has_designator_check when first_initializer_p
or for the inner constructor element." would be better.

>         (cp_complete_array_type): do reshape_init on braced-init-list.

s/do/Do/

> gcc/testsuite/ChangeLog:
> 
>         * g++.dg/cpp2a/desig20.C: New test.
> ---
>  gcc/cp/decl.c                        | 28 ++++++----------
>  gcc/testsuite/g++.dg/cpp2a/desig20.C | 48 ++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 19 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp2a/desig20.C
> 
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 947bbfc6637..f01655c5c14 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -6820,6 +6820,7 @@ reshape_init_r (tree type, reshape_iter *d, tree first_initializer_p,
>      {
>        tree str_init = init;
>        tree stripped_str_init = stripped_init;
> +      reshape_iter stripd = {};

Since the previous variables spell it "stripped" maybe call it stripped_iter.
  
>        /* Strip one level of braces if and only if they enclose a single
>  	 element (as allowed by [dcl.init.string]).  */
> @@ -6827,7 +6828,8 @@ reshape_init_r (tree type, reshape_iter *d, tree first_initializer_p,
>  	  && TREE_CODE (stripped_str_init) == CONSTRUCTOR
>  	  && CONSTRUCTOR_NELTS (stripped_str_init) == 1)
>  	{
> -	  str_init = (*CONSTRUCTOR_ELTS (stripped_str_init))[0].value;
> +	  stripd.cur = CONSTRUCTOR_ELT (stripped_str_init, 0);
> +	  str_init = stripd.cur->value;
>  	  stripped_str_init = tree_strip_any_location_wrapper (str_init);
>  	}
>  
> @@ -6836,7 +6838,8 @@ reshape_init_r (tree type, reshape_iter *d, tree first_initializer_p,
>  	 array types (one value per array element).  */
>        if (TREE_CODE (stripped_str_init) == STRING_CST)
>  	{
> -	  if (has_designator_problem (d, complain))

So the logic here is that...

> +	  if ((first_initializer_p && has_designator_problem (d, complain))

this will complain about a designator in the outermost { }:

  char arr[] = { [0] = "foo" };

and...

> +	      || (stripd.cur && has_designator_problem (&stripd, complain)))

this checks the { } which is at least one level deep, contains a STRING_CST,
and this line makes sure that we don't have a designator that would refer to
some element of the array we're initializing with the STRING_CST, yes?  I.e.,
something like

  struct S { char arr[10]; };
  S s = { { [2] = "lol" } };

>  	    return error_mark_node;
>  	  d->cur++;
>  	  return str_init;
> @@ -9538,23 +9541,10 @@ cp_complete_array_type (tree *ptype, tree initial_value, bool do_default)
>  
>    if (initial_value)
>      {
> -      /* An array of character type can be initialized from a
> -	 brace-enclosed string constant.

Nice to finally remove this, but let's keep this part of the comment.

> -	 FIXME: this code is duplicated from reshape_init. Probably
> -	 we should just call reshape_init here?  */
> -      if (char_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (*ptype)))
> -	  && TREE_CODE (initial_value) == CONSTRUCTOR
> -	  && !vec_safe_is_empty (CONSTRUCTOR_ELTS (initial_value)))
> -	{
> -	  vec<constructor_elt, va_gc> *v = CONSTRUCTOR_ELTS (initial_value);
> -	  tree value = (*v)[0].value;
> -	  STRIP_ANY_LOCATION_WRAPPER (value);
> -
> -	  if (TREE_CODE (value) == STRING_CST
> -	      && v->length () == 1)
> -	    initial_value = value;
> -	}
> +      if (TREE_CODE (initial_value) == CONSTRUCTOR
> +	  && BRACE_ENCLOSED_INITIALIZER_P (initial_value))

BRACE_ENCLOSED_INITIALIZER_P checks that it gets a CONSTRUCTOR so you
can remove the first check.

> +	initial_value = reshape_init (*ptype, initial_value,
> +				      tf_warning_or_error);
>  
>        /* If any of the elements are parameter packs, we can't actually
>  	 complete this type now because the array size is dependent.  */
> diff --git a/gcc/testsuite/g++.dg/cpp2a/desig20.C b/gcc/testsuite/g++.dg/cpp2a/desig20.C
> new file mode 100644
> index 00000000000..03eab764325
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp2a/desig20.C
> @@ -0,0 +1,48 @@
> +// PR c++/55227 
> +// Test designated initializer for char array by string constant
> +
> +// { dg-do compile }
> +// { dg-options "-pedantic" }

FWIW, if you remove the dg-options, -pedantic-errors will be used so you could
drop it and then use dg-error instead of dg-warning below but this is OK too.

> +
> +struct C {char a[2];};
> +
> +/* Case a; designated, unbraced, string-literal of the same size as the target
> +   char array to be initialized; valid and accepted before and after.  */
> +
> +C a = {.a="a"}; // { dg-warning "designated initializers only available with" "" { target c++17_down } .-0 }

.-0 looks weird and you don't need it, just drop it.

We should probably test more:
- nested structs
- anonymous unions
- test when the initializer is too long
- multidim arrays:

  char a[][10] = { { "aaa" } };
  char a2[][10] = { [0] = { "aaa" } };

They all appear to work, so it's just about extending the test.

Hope this is useful...

Marek