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):


+      /* 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 : 
+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.


Reply via email to