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