This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 07/23/2016 01:18 PM, Martin Sebor wrote:
+  /* A pair of the first non-static non-empty data members following
+     either the flexible array member, if found, or the zero-length
+     array member otherwise.  AFTER[1] refers to the first such data
+     member of a union that the struct containing the flexible array
+     member or zero-length array is a member, or NULL when no such
+     union exists.  AFTER[0] refers to the first such data member
+     that is not a member of such a union.  */

This is pretty hard to follow, could you add an example? Why do we want to track these separately rather than look at DECL_CONTEXT at diagnostic time?

+  /* The type in which an anonymous struct or union containing ARRAY
+     is defined or null if no such anonymous struct or union exists.  */
+  tree anonctx;

It seems clearer to find this at diagnostic time by following TYPE_CONTEXT.

      /* Find the next non-static data member if it exists.  */
      for (next = fld;
           (next = DECL_CHAIN (next))
             && TREE_CODE (next) != FIELD_DECL; );

I think next_initializable_field would be useful here.

       if (TREE_CODE (fld) != TYPE_DECL
          && RECORD_OR_UNION_TYPE_P (fldtype)
-         && TYPE_ANONYMOUS_P (fldtype))
+         && VAR_DECL != TREE_CODE (fld)
+         && (FIELD_DECL != TREE_CODE (fld) || !DECL_FIELD_IS_BASE (fld)))

Please put the constant on the RHS of comparisons.

It seems that you're only interested in FIELD_DECL here, so maybe move up the

      /* Skip anything that's not a (non-static) data member.  */
      if (TREE_CODE (fld) != FIELD_DECL)
        continue;

and remove the checks for TYPE_DECL and VAR_DECL? For that matter, you could also move up the DECL_ARTIFICIAL check so you don't need to check DECL_FIELD_IS_BASE.

+         /* Is the member an anonymous struct or union?  */
+         bool anon_p = (!TYPE_ANONYMOUS_P (fldtype)
+                        || !DECL_NAME (fld)
+                        || anon_aggrname_p (DECL_NAME (fld)));

This looks like anon_p will be true for any non-static data member of non-anonymous type? Maybe you want ANON_AGGR_TYPE_P?

+	  /* If this member isn't anonymous and a prior non-flexible array
+	     member has been seen in one of the enclosing structs, clear
+	     the FIRST member since it doesn't contribute to the flexible
+	     array struct's members.  */
+	  if (first && !array && !anon_p)
+	    fmem->first = NULL_TREE;

Why is this needed? For a non-anonymous member, presumably we'll give an error when analyzing the type of the member on its own, so we shouldn't need to complain again here.

              /* Replace the zero-length array if it's been stored and
                 reset the after pointer.  */
              if (TYPE_DOMAIN (TREE_TYPE (fmem->array)))
                {
                  fmem->after[bool (pun)] = NULL_TREE;
                  fmem->array = fld;
                }

So we silently allow a zero-length array followed by a flexible array?  Why?

+	  msg = G_("zero-size array member %qD in an otherwise empty %q#T");
+
+	if (msg && pedwarn (DECL_SOURCE_LOCATION (fmem->array),
+			    OPT_Wpedantic, msg, fmem->array, fmemctx))
+	  {
+	    inform (location_of (t), "in the definition of %q#T", fmemctx);
+
+	    /* Prevent the same flexible array member from being diagnosed
+	       more than once if it happens to be nested in more than one
+	       union and overlap with another member.  This avoids multiple
+	       warnings for perverse cases like the the following where
+	       U::U1::X::a1 would otherwise be diagnosed first followed by
+	       S::U1::X::a1:
+	         struct S {
+	           union U {
+	             union U1 { struct X { int n1, a1[]; } x1; } u1;
+		     union U2 { struct X { int n2, a2[]; } x1; } u2;
+		   } u;
+		 } s;
+	    */
+	    TREE_NO_WARNING (fmem->array) = 1;
+	  }

There doesn't seem to be anything that checks TREE_NO_WARNING for the zero-size array case.

+	    /* Avoid issuing further duiagnostics after the error above.  */

Typo.

  if (fmem == &flexmems
      && !TYPE_ANONYMOUS_P (t) && !anon_aggrname_p (TYPE_IDENTIFIER (t)))

I think you want ANON_AGGR_TYPE_P here, too.

I also wonder about integrating this with layout_class_type, since that is also looking at layout issues.

Jason


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]