[PATCH] D30766: Add support for attribute "enum_extensibility"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

Aside from the request for a FIXME and a decision from the author on error vs warning, the code LGTM. Feel free to make a decision and commit without further review.



================
Comment at: test/Sema/enum-attr.c:27
+
+enum __attribute__((enum_extensibility(arg1))) EnumInvalidArg { // expected-warning{{'enum_extensibility' attribute argument not supported: 'arg1'}}
+  G
----------------
ahatanak wrote:
> aaron.ballman wrote:
> > ahatanak wrote:
> > > arphaman wrote:
> > > > Should this be an error instead?
> > > Yes, I agree.
> > I'm not opposed to it, but we do treat it as a warning for every other attribute (and just ignore the attribute), FWIW.
> Do you know the reason we treat wrong attribute arguments as a warning in other cases rather than an error?
> 
> I'm guessing a warning is preferred because in most cases dropping an attribute doesn't affect correctness (it doesn't cause miscompiles).
You are correct. The general rule is to use warnings when an incorrect attribute still has no impact on the code generated for the user, and use an error otherwise. We're not always perfect about this, which is why I'm not opposed, but since this involves adding an entirely new diagnostic, I figured it might be worth mentioning that there's really no reason for this to be an error that prevents the user's source code from being translated either.


================
Comment at: test/Sema/enum-attr.c:31
+
+enum __attribute__((enum_extensibility(closed,open))) EnumTooManyArgs { // expected-error{{use of undeclared identifier 'open'}}
+  X1
----------------
That's a rather unfortunate diagnostic. I would have expected this to mention that it only takes one argument as in the zero arg case. I'm not certain that this is the fault of your patch, however. Can you at least add a FIXME to this test that mentions this diagnostic could be improved?


https://reviews.llvm.org/D30766