Re: [PATCH] accept flexible arrays in struct in unions (c++/71912 - [6/7 regression])

[Jason Merrill <jason at redhat dot com>]

On 09/14/2016 01:03 PM, Martin Sebor wrote:
> Attached is an updated patch that does away with the "overlapping"
> warning and instead issues the same warnings as the C front end
> (though with more context).
>
> In struct flexmems_t I've retained the NEXT array even though only
> the first element is used to diagnose problems.  The second element
> is used by the find_flexarrays function to differentiate structs
> with flexible array members in unions (which are valid) from those
> in structs (which are not).
>
> FWIW, I think this C restriction on structs is unnecessary and will
> propose to remove it so that's valid to define a struct that contains
> another struct (possibly recursively) with a flexible array member.
> I.e., I think this should be made valid in C (and accepted without
> the pedantic warning that GCC, and with this patch also G++, issues):

Agreed.

> +      /* Is FLD a typedef for an anonymous struct?  */
> +      bool anon_type_p
> +       = (TREE_CODE (fld) == TYPE_DECL
> +          && DECL_IMPLICIT_TYPEDEF_P (fld)
> +          && anon_aggrname_p (DECL_NAME (fld)));

We talked earlier about handling typedefs in 
cp_parser_{simple,single}_declaration, so that we catch

typedef struct { int a[]; } B;

or at least adding a FIXME comment here explaining that looking at 
typedefs is a temporary hack.

> +      /* Type of the member.  */
> +      tree fldtype = TREE_CODE (fld) == FIELD_DECL ? TREE_TYPE (fld) : fld;

Why set "fldtype" to be a TYPE_DECL rather than its type?

> +      /* Determine the type of the array element or object referenced
> +        by the member so that it can be checked for flexible array
> +        members if it hasn't been yet.  */
> +      tree eltype = TREE_CODE (fld) == FIELD_DECL ? fldtype : TREE_TYPE (fld);

Given the above, this seems equivalent to

tree eltype = TREE_TYPE (fld);

> +      if (RECORD_OR_UNION_TYPE_P (eltype))
> +       {
...
> +         if (fmem->array && !fmem->after[bool (pun)])
> +           {
> +             /* Once the member after the flexible array has been found
> +                we're done.  */
> +             fmem->after[bool (pun)] = fld;
> +             break;
> +           }
...
>       if (field_nonempty_p (fld))
>         {
...
>           /* Remember the first non-static data member after the flexible
>              array member, if one has been found, or the zero-length array
>              if it has been found.  */
>           if (fmem->array && !fmem->after[bool (pun)])
>             fmem->after[bool (pun)] = fld;
>         }

Can we do this in one place rather than two?

> +         if (eltype == fldtype || TYPE_UNNAMED_P (eltype))

This is excluding arrays, so we don't diagnose, say,

> struct A
> {
>   int i;
>   int ar[];
> };
>
> struct B {
>   A a[2];
> };

Should we complain elsewhere about an array of a class with a flexible 
array member?

> +             /* Does the field represent an anonymous struct?  */
> +             bool anon_p = !DECL_NAME (fld) && ANON_AGGR_TYPE_P (eltype);

You don't need to check DECL_NAME; ANON_AGGR_TYPE_P by itself indicates 
that we're dealing with an anonymous struct/union.

>    Similarly, PSTR is set to the outermost struct of which T is a member
>    if one such struct exists, otherwise to NULL.  */
...
>               find_flexarrays (eltype, fmem, false, pun,
>                                !pstr && TREE_CODE (t) == RECORD_TYPE ? fld : pstr);
...
> +diagnose_invalid_flexarray (const flexmems_t *fmem)
> +{
> +  if (fmem->array && fmem->enclosing
> +      && pedwarn (location_of (fmem->enclosing), OPT_Wpedantic,
> +                 TYPE_DOMAIN (TREE_TYPE (fmem->array))
> +                 ? G_("invalid use of %q#T with a zero-size array "
> +                      "in %q#D")
> +                 : G_("invalid use of %q#T with a flexible array member "
> +                      "in %q#T"),
> +                 DECL_CONTEXT (fmem->array),
> +                 DECL_CONTEXT (fmem->enclosing)))

PSTR is documented to be the outermost struct, but it (and thus 
fmem->enclosing) end up being a data member of that outermost struct, 
which is why you need to take its DECL_CONTEXT.  What's the advantage of 
doing that over passing t itself to the recursive call?

> +  /* The context of the flexible array member.  Either the struct
> +     in which it's declared or, for anonymous structs and unions,
> +     the struct/union of which the array is effectively a member.  */
> +  tree fmemctx = anon_context (fmem->array);
> +  bool anon_p = fmemctx != DECL_CONTEXT (fmem->array);
> +  if (!anon_p)
> +    fmemctx = t;

Why do you need to do something different here based on anon_p?  I don't 
see why that would affect whether we want to look through intermediate 
non-anonymous classes.

> +      check_flexarrays (basetype, fmem, true);

Please decorate boolean literal arguments like this with the name of the 
parameter, e.g. /*base_p*/true.

> +  bool maybe_anon_p = anon_aggrname_p (TYPE_IDENTIFIER (t));

Now we can use TYPE_UNNAMED_P.

Jason