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
