[cfe-dev] Fwd: Review of transparent_union patch

[Anders Johnsen]

Hi,

Thank you for the review. I've attached a corrected patch.

Anders

On Fri, Jan 23, 2009 at 7:43 PM, Douglas Gregor <dgregor at apple.com> wrote:
> 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
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: transparent_union.patch
Type: text/x-diff
Size: 8455 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090123/2743c743/attachment.patch>