This is an archive of the discontinued LLVM Phabricator instance.

Transparent_union attribute should be possible in front of union (rework)
ClosedPublic

Authored by erichkeane on Jan 3 2017, 4:48 PM.

Details

Summary

This is an update of this patch: https://reviews.llvm.org/D25824

I cannot seem to get that review to update with my diff. This first patch removes the dependency on isParsing and switches to isBeingDefined. In order to get that to work, changing where attributes where changed was necessary.

Original description below:

Clang compiles with error following test case

typedef union attribute((transparent_union)) {

int *i;
struct st *s;

} TU;

void bar(TU);

void foo(int *i) {

bar(i);

}

clang -c tu.c
tu.c:1:30: warning: transparent_union attribute can only be applied to a union definition; attribute ignored [-Wignored-attributes]
typedef union attribute((transparent_union)) {

^

tu.c:8:24: error: passing 'int *' to parameter of incompatible type 'TU'
void foo(int *i) { bar(i); }

^

tu.c:6:12: note: passing argument to parameter here
void bar(TU);

^

GCC compiles this test successfully.

The compilation is failed because the routine handleTransparentUnionAttr requires for the record decl to be completed. This fix provides handling of the attribute ‘transparent_union’ after parsing of union.

Diff Detail

Repository
rL LLVM

Event Timeline

erichkeane updated this revision to Diff 82976.Jan 3 2017, 4:48 PM
erichkeane retitled this revision from to Transparent_union attribute should be possible in front of union (rework).
erichkeane updated this object.
erichkeane added inline comments.
lib/Sema/SemaDecl.cpp
13499 ↗(On Diff #82976)

I wasn't sure if this was the correct change in here. My other option was to move the above 2 lines (13495/13496) above these line's original spot, but moving it here seems to have correctly passed all tests. Anyone with more knowledge here want to chime in?

test/Sema/transparent-union.c
57 ↗(On Diff #82976)

2 Additional sema tests to ensure that the same warnings occur both in prefix and postfix attribute definition.

vbyakovlcl removed a subscriber: vbyakovlcl.

Hi guys-
Since this is a rework of Vlad's changes, I'm not sure you noticed that this was a different review! Anyone have opinions? Additionally, I had 1 question in SemaDecl.cpp that I think was right, otherwise I hope this is a pretty light review.

aaron.ballman edited edge metadata.Feb 27 2017, 1:17 PM

Some minor nits, but I think this is generally correct.

include/clang/Sema/Sema.h
3059 ↗(On Diff #82976)

I would remove the mention of TransparentUnion -- this is generalized for other uses already.

lib/Parse/ParseDeclCXX.cpp
1889 ↗(On Diff #82976)

I'd remove mention of TransparentUnion here as well.

lib/Sema/SemaDecl.cpp
13499 ↗(On Diff #82976)

I think this change is reasonable.

lib/Sema/SemaDeclAttr.cpp
6201 ↗(On Diff #82976)

Nit: the * should bind to l and l should be L by our usual naming conventions (but a name better than L wouldn't be amiss).

erichkeane marked 6 inline comments as done.Feb 28 2017, 10:21 AM

Thanks! Changes incoming.

Response to Aaron's comments

aaron.ballman accepted this revision.Feb 28 2017, 12:44 PM

LGTM, with one formatting nit.

lib/Sema/SemaDeclAttr.cpp
6201 ↗(On Diff #82976)

Nit: missing a space before the first =. You should run clang-format on the patch to catch this sort of thing.

This revision is now accepted and ready to land.Feb 28 2017, 12:44 PM
This revision was automatically updated to reflect the committed changes.