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:
> > > 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.
> On the whole, this makes me more comfortable with the idea, though some
> questions remain.
>
> How do you envision this working with custom subjects, like `tls_model` which
> only applies to variables with thread-local storage?
>
> I see two options there:
>
> (0) The user has to use a specific "strict" entity, like tls_var. This seems
> unlikely to scale well, unless we automate the generation and checking for
> those entities. It also seems somewhat user-hostile because users are
> unlikely to be able to guess what to write for that subject.
>
> (1) The user specifies the base subject kind with no extra requirements
> (i.e., variables, but we don't care if they're thread-local). It would still
> be somewhat fragile because it means that custom subject lines are more
> subject to inadvertently changing the user's code, but it also means the user
> doesn't need to keep an arbitrary list of weird attribute subjects in their
> head.
>
> The code completion and fix-it support would go a long ways towards helping
> with either choice.
>
> How would both modes work for attributes without any Subjects list at all? I
> could imagine the non-strict mode simply attempting to apply the attribute to
> whatever subjects are listed, and the compiler would diagnose anything that
> was incorrect. e.g.,
> ```
> #pragma clang attribute push([[gnu::cdecl]], apply_only_to={variables})
> int x; // Gets diagnosed as a bad attribute subject.?
> ```
> Perhaps if there's no Subjects list, there's no strict-mode support
> (attempting use it is diagnosed as an error)?
> How do you envision this working with custom subjects, like tls_model which
> only applies to variables with thread-local storage?
>
> I see two options there:
>
> (0) The user has to use a specific "strict" entity, like tls_var. This seems
> unlikely to scale well, unless we automate the generation and checking for
> those entities. It also seems somewhat user-hostile because users are
> unlikely to be able to guess what to write for that subject.
>
> (1) The user specifies the base subject kind with no extra requirements
> (i.e., variables, but we don't care if they're thread-local). It would still
> be somewhat fragile because it means that custom subject lines are more
> subject to inadvertently changing the user's code, but it also means the user
> doesn't need to keep an arbitrary list of weird attribute subjects in their
> head.
It's tough to pick a one or the other, but in general I think we should make it
more specific. I think we can try a syntax that's similar to clang's
ASTMatchers:
e.g.
```
#pragma clang attribute push(tls_model, apply_to = { variable(isThreadLocal())
})
```
>
> The code completion and fix-it support would go a long ways towards helping
> with either choice.
>
> How would both modes work for attributes without any Subjects list at all? I
> could imagine the non-strict mode simply attempting to apply the attribute to
> whatever subjects are listed, and the compiler would diagnose anything that
> was incorrect. e.g.,
>
> #pragma clang attribute push([[gnu::cdecl]], apply_only_to={variables})
> int x; // Gets diagnosed as a bad attribute subject.?
> Perhaps if there's no Subjects list, there's no strict-mode support
> (attempting use it is diagnosed as an error)?
Initially we will only allow attributes that a have subject list (except
annotate). But for something like annotate, we should prohibit the strict
version (`apply_to`) and only offer the non-strict version `apply_only_to`.
Repository:
rL LLVM
https://reviews.llvm.org/D30009
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
