[cfe-dev] Review of transparent_union patch

[Douglas Gregor]

Anders Johnsen submitted a patch to implement semantic analysis for  
transparent_union in

	http://llvm.org/bugs/show_bug.cgi?id=2015

Here's a review of that patch, which is almost ready to go in.

Could you include a test-case as part of your patch?

+  // FIXME: Corredt behaviour for no-definition unions?
+  if (!RD->isDefinition())
+    return;

Typo "corredt".

Also, I don't think this is the behavior we want. If the union is
forward-declared with the transparent_union attribute, we should keep
that attribute, as in this example:

   union wait { float *fp; double *dp; };

   union wait_status_ptr_t  __attribute__ ((__transparent_union__));

   union wait_status_ptr_t
   {
     int *__ip;
     union wait *__up;
   };

   int wait (union wait_status_ptr_t);

   int w1 () { int w; return wait (&w); }
   int w2 () { union wait w; return wait (&w); }

GCC fails on this example. We also fail, because when there is no
definition of the union, we silently ignore the attribute. We either
need to (a) produce a warning saying that this attribute is ignored if
it isn't on the definition, or (b) add the attribute, then make sure
it gets processed properly when the union is defined, later. While I
like (b) better, GCC seems to be doing (a) without warning about it,
so I suggest we do (a) with the warning.

+  // Ignore anonymous unions.
+  if (RD->isAnonymousStructOrUnion())
+    return;

Here, again, we should produce a warning saying that we're ignoring
this attribute, since it can't apply to anonymous structs/unions.

+  if (ArgType->isUnionType()) {
+    const RecordType *UT = ArgType->getAsUnionType();

Please replace this with:

   if (const RecordType *UT = ArgType->getAsUnionType())

That way, we only walk through typedefs once.

GCC completely ignores the transparent_union attribute in C++ mode. I
suggest that we do the same (and produce a warning to say we did it!).

With those (few) tweaks, this is ready to go in. Thanks!

   - Doug