[PATCH] D30009: Add support for '#pragma clang attribute'

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:308-311
+  // - An attribute requires delayed parsing (LateParsed is on)
+  // - An attribute has no GNU/CXX11 spelling
+  // - An attribute has no subject list
+  // - An attribute derives from a StmtAttr or a TypeAttr
----------------
arphaman wrote:
> aaron.ballman wrote:
> > I have strong reservations about this -- users are going to have no idea what attributes are and are not supported because they're not going to know whether the attribute has a subject list or requires delayed parsing. We have a considerable number of attributes for which the Subjects line is currently commented out simply because no one has bothered to fix that. This means those attributes can't be used with this pragma until someone fixes that, but when it happens, they magically can be used, which is a good thing. But the converse is more problematic -- if there's an existing Subjects line that gets removed because a subject is added that is difficult to express in TableGen it may break user code.
> > 
> > We can fix the discoverability issues somewhat by updating the documentation emitter to spit out some wording that says when an attribute is/is not supported by this feature, but that only works for attributes which have documentation and so it's not a particularly reliable workaround.
> > I have strong reservations about this -- users are going to have no idea what attributes are and are not supported because they're not going to know whether the attribute has a subject list or requires delayed parsing. We have a considerable number of attributes for which the Subjects line is currently commented out simply because no one has bothered to fix that. This means those attributes can't be used with this pragma until someone fixes that, but when it happens, they magically can be used, which is a good thing. But the converse is more problematic -- if there's an existing Subjects line that gets removed because a subject is added that is difficult to express in TableGen it may break user code.
> 
> That's a good point. I think we can address this problem by creating a test that verifies the list of attributes that support the pragma. This would allow us to ensure that no attributes loose the ability to use the pragma.
> 
> > We can fix the discoverability issues somewhat by updating the documentation emitter to spit out some wording that says when an attribute is/is not supported by this feature, but that only works for attributes which have documentation and so it's not a particularly reliable workaround.
> 
> We can enforce the rule that the attributes can only be used with '#pragma clang attribute' when they're documented. This way we can ensure that all attributes that can be used with the pragma are explicitly documented.
> That's a good point. I think we can address this problem by creating a test that verifies the list of attributes that support the pragma. This would allow us to ensure that no attributes loose the ability to use the pragma.

That would be good.

> We can enforce the rule that the attributes can only be used with '#pragma clang attribute' when they're documented. This way we can ensure that all attributes that can be used with the pragma are explicitly documented.

This addresses the concern about discoverability, but it worsens the concerns about fragility. The biggest problem is: the user has very little hope of understanding what attributes will apply to what declarations with which version of the compiler they're using. With this sort of thing, the act of us adding documentation can impact the behavior of a user's program from one release to the next.

While I can imagine this pragma reducing some amount of code clutter, it is far too "magical" for me to feel comfortable with it (at least in the proposed form). I cannot read the user's source code and understand what attributes are going to be applied to which declarations, and that's not a good story for usability of the feature.


Repository:
  rL LLVM

https://reviews.llvm.org/D30009