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

Hi,

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.
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.

I'll be happy to ear your thoughts...

Pierre.

[1] Unnamed Fields - Using the GNU Compiler Collection (GCC)

AllowUnnamedStructOrUnionInStructOrUnion.patch (3.91 KB)

It looks generally fine to me. It's probably a good idea for someone
else to review it, though.

A couple of small comments:

1. Do we want to support this extension in gnu89 mode?
2. Does this correctly deal with "struct foo {int a,b;}; struct bar
{int a; struct foo;};" and "struct bar {int a; struct foo {int
a,b;};};"?
3. For "struct { int a; union { int a; } }", can we produce an error?
The gcc docs seem to imply that's it's illegal, and it's usually
better to produce an error where we can.

-Eli

Hi Pierre,

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

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.

The option name "AllowUnnamedStructOrUnionInStructOrUnion" is really,
really long :slight_smile:
I'd be inclined to remove this option. Instead, just allow this
extension unless NoExtensions is set. On a related note, anonymous
unions (but anonymous structs aren't) are legal in C++, so the patch
should reflect that.

  - Doug

Eli,

Hi,

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.
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.

I'll be happy to ear your thoughts...

It looks generally fine to me. It's probably a good idea for someone
else to review it, though.

A couple of small comments:

1. Do we want to support this extension in gnu89 mode?

Right.

The docs does not state that explicitly, yet it is supported.

$ gcc -c -std=gnu89 test/Sema/unnamed-struct-in-struct.c
$

Doug suggested to do that always unless NoExtensions is specify this is what I've done... Yet, -std=c99 doesn't enable NoExtensions... Weird.

2. Does this correctly deal with "struct foo {int a,b;}; struct bar
{int a; struct foo;};"

Well, nice catch as well:

$ gcc -c test-2.c
test-2.c:2: warning: declaration does not declare anything
test-2.c: In function 'dummy':
test-2.c:8: error: 'struct bar' has no member named 'b'
$

Previous version enables such an anonymous struct usage. Attached patch filters this out as well.

"struct bar {int a; struct foo {int a,b;};};"?

$ gcc -c test-3.c
test-3.c:1: warning: declaration does not declare anything
test-3.c: In function 'dummy':
test-3.c:6: error: 'struct bar' has no member named 'b'
$

Previous version enables such an anonymous struct usage. Attached patch filters this out as well.

3. For "struct { int a; union { int a; } }", can we produce an error?
The gcc docs seem to imply that's it's illegal, and it's usually
better to produce an error where we can.

Actually current gcc does not produce error as stated in the doc. Yet, I agree that this would be way better to output error.

Patch is attached to next email in this thread.

Pierre.

Hi Doug,

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?

0001-Allow-unnamed-structure-or-union-fields-within-struc.patch (20.2 KB)