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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman 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
----------------
aaron.ballman wrote:
> 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.
What about the following idea:

The user has to include a **strict** set of declarations that receive the attribute explicitly, e.g.:

```
#pragma clang attribute push([[noreturn]], apply_to={ function })
``` 

The compiler then would verify that the set of declarations (in this case just `function`) is **strictly** identical to the built-in compiler-defined set of declarations that can receive the attribute (i.e. the strict set has to include all of the supported declarations). This will ensure that the user will know what declarations receive the attribute. If the compiler changes the set of allowed attributes in the future, e.g. if clang can now apply `[[noreturn]]` to enums for some reason, the user would get a compilation error saying something like `the [[noreturn]] attribute also applies to enum declarations` with a fix-it that inserts the `, enum`.

The compiler would also provide code-completion and fix-it support that insert the default strict set into the pragma. 

Then it would also provide an additional mode in which the explicitly specified list of declarations doesn't have to be strictly identical, but it still should be a subset of declarations that can receive the attribute, e.g.

```
#pragma clang attribute push(internal_linkage, apply_only_to={ function })
``` 

Clang could then extend the `internal_linkage` attribute so that it would apply to enums, which wouldn't trigger a compilation error since the set of declarations that receive the attribute isn't strict. However, the user would get a compilation error if clang removed  `function` from the list of supported declarations.


Repository:
  rL LLVM

https://reviews.llvm.org/D30009