[cfe-dev] Fwd: Review of transparent_union patch

[Anders Johnsen]

Hi,

On Fri, Jan 23, 2009 at 10:08 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> On Fri, Jan 23, 2009 at 11:33 AM, Anders Johnsen <skabet at gmail.com> wrote:
>> Hi,
>>
>> Thank you for the review. I've attached a corrected patch.
>
> A few notes:
>
> 1. This breaks CodeGen for transparent unions, which was working
> before; it seems like this will be a large regression for CodeGen for
> many programs.

Yes. I'm not sure where in CodeGen I should add support for this. But
the other way was limited to pointer-types only and was using the
wrong CC(should be as first type *i8, but was handled as [x * i8]
afaik.

>
> 2. The way the attribute is getting attached looks wrong; this is a
> union attribute, so it's required to appear immediately after the
> definition of the union.  I suppose that's mostly orthogonal to this
> patch, though.

What exactly do you mean by this?
>
> 3.
> +  // Ignore anonymous unions.
> +  if (RD->isAnonymousStructOrUnion()) {
> +    S.Diag(Attr.getLoc(),
> +        diag::warn_transparent_union_attribute_anonymous);
> +    return;
> +  }
> Why exactly are we ignoring anonymous structs/unions?  gcc seems to
> support them; here's a testcase:
> void a(union {int a; int* b;} __attribute((transparent_union)) x);
> void b() {a(1);}

You are right. Fixed

>
> 4. It looks like this allows floating-point types where gcc doesn't; testcase:
>
> union x {float a;int* b;} __attribute((transparent_union));
> void a(union x a);
> void b() {a(1.0f);}
>
> -Eli
>

I've not handled the case where the first element is a floating point
or vector. Is fixed in patch.

I've also added a check for a empty union.

Thanks you for the comments!

Anders
-------------- next part --------------
A non-text attachment was scrubbed...
Name: transparent_union.patch
Type: text/x-diff
Size: 8822 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090123/9a9cd9f5/attachment.patch>