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

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked an inline comment as done.
ahatanak added inline comments.


================
Comment at: test/Sema/enum-attr.c:27
+
+enum __attribute__((enum_extensibility(arg1))) EnumInvalidArg { // expected-warning{{'enum_extensibility' attribute argument not supported: 'arg1'}}
+  G
----------------
aaron.ballman wrote:
> 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.
OK. I don't think we have a strong reason to error out here so I think it's better to follow convention and issue a warning.


================
Comment at: test/Sema/enum-attr.c:31
+
+enum __attribute__((enum_extensibility(closed,open))) EnumTooManyArgs { // expected-error{{use of undeclared identifier 'open'}}
+  X1
----------------
aaron.ballman wrote:
> 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?
It's possible to implement a parser function in ParseDecl.cpp specifically for enum_extensibility to improve the diagnostic, but I think ParseAttributeArgsCommon should be able to check the number of arguments and emit a proper diagnostic. I've added a FIXME to the test for now.

It looks like ParseAttributeArgsCommon currently assumes that the first argument can be an identifier but it tries to parse the remaining arguments as expressions. I think it's possible to make it check the number of arguments provided and parse each argument according to its type.


https://reviews.llvm.org/D30766