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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
