[cfe-dev] [PATCH] Allow unnamed structure or union fields within structs/unions in gnu99 mode

[Pierre d'Herbemont]

Hi Doug,

On Nov 10, 2008, at 4:32 PM, Doug Gregor wrote:

> On Sun, Nov 9, 2008 at 11:53 AM, Pierre d'Herbemont <pdherbemont at free.fr 
> > wrote:
>> This patch enables unnamed structure or union fields withing  
>> structs/unions
>> when -std=gnu99 is used. Cf [1].
>>
>> Note, as stated in [1], struct { int a; union { int a; } } is  
>> discouraged,
>> but won't produce any error nor warning.
>
> As Eli said, this patch should really produce an error

Latest patch has something that checks for error now.

>> It is unclear for me that we really want to add anonymous structure  
>> support
>> in RecordDecl::getMember, as implementation could vary given the  
>> language
>> standard. Yet, not implementing here could lead to easy and  
>> unoticeable
>> mistakes if the caller doesn't implement anonymous structure  
>> resolving.
>
> Yeah, this is an interesting issue... what happens if you try to
> actually generate code for your example program? And execute the
> function dummy()? Since we have code-generation support for most of C,
> I'd like for implementation of new C features to support
> code-generation from the start. Maybe it will just work, but I'm
> guessing that if it *doesn't* work, we may have to consider extending
> the AST more to make it work.

Well, I didn't figure that out at first. Latest patch fixes that. This  
time it doesn't changes the AST, instead it adds fakely create the  
missing MemberExpr to navigate in the anonymous fields.

An other solution involves extending the MemberExpr, but this requires  
more changes on clients (like CodeGen). What do you prefer?

--
After writing this mail I saw Patrick Walton patch... :)

I guess we could merge the two patches, as Patrick's modification to  
the AST seems to be the preferred solution. I am still attaching the  
patch before merge.
--

> The option name "AllowUnnamedStructOrUnionInStructOrUnion" is really,
> really long :)

I like long explicit name :) But it's now gone...

> I'd be inclined to remove this option. Instead, just allow this
> extension unless NoExtensions is set.

Ok, this is what is done in latest patch. Yet with -std=c99,  
NoExtensions doesn't seem to be set.

Though, we'll get warning with -pedantic.

> On a related note, anonymous
> unions (but anonymous structs aren't) are legal in C++, so the patch
> should reflect that.

It should now :)

In this patch:
- The Expr tree is expanded for the unnamed struct/union/class (An  
other solution is Patrick Walton's patch :) )
- We produce errors on field redefinitions such as "struct foo { int  
a; union { float a; int b} };"
- "struct a { struct foo; struct bar {}; }" is correctly filtered
- "struct { int a; }" outside of a struct/union/class is correctly  
filtered.
- Specific hack for the obj-c case property that should not allow  
anonymous structs when generating objc properties.
- llvm code generation seems correct.
- All clang tests pass

Things I am not proud of:
- getStructOrUnionIdentifierLoc(), the name is probably badly chosen,  
but that's how I manage to filter out "struct a { struct foo; }"

I'll be happy to ear your feedbacks on that one as well.

Pierre.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Allow-unnamed-structure-or-union-fields-within-struc.patch
Type: application/octet-stream
Size: 20644 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081115/d3703a0b/attachment.obj>
-------------- next part --------------