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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D30009#1218669, @dexonsmith wrote:

> In https://reviews.llvm.org/D30009#1218647, @rsmith wrote:
>
> > If we really want an "only documented attribtues can use this feature" rule 
> > (and I'm really not convinced that is a useful rule to enforce
>
>
> I also don't quite see why this is useful.


OK, https://reviews.llvm.org/D51507 sent out for review to remove the rule.


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

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

[Aaron Ballman via cfe-commits]

On Wed, Aug 29, 2018 at 8:41 PM, Richard Smith - zygoloid via
Phabricator  wrote:
> rsmith added a comment.
> Herald added a subscriber: llvm-commits.
>
> One consequence of this patch (see https://reviews.llvm.org/rL341002) is that 
> adding documentation to an existing attribute //changes the behavior of 
> Clang//. That does not seem acceptable to me: documentation improvements 
> should not change which attributes we allow this pragma to appertain to.
>
> If we really want an "only documented attribtues can use this feature" rule 
> (and I'm really not convinced that is a useful rule to enforce), I think the 
> best way to do that is to add another flag on the Attr instance in the .td 
> file to opt the attribute out of `#pragma clang` support, opt out all the 
> undocumented attributes, and add an error to TableGen if we find that an 
> undocumented attribute has `#pragma clang` support enabled.

I think this is a reasonable approach, but I'm also not as convinced
we need the attributes to be documented in order to use this pragma
any more. The thought process behind why it was implemented this way
(from memory, so forgive me if I'm fuzzy) was because a blanket
application of an attribute through a pragma makes for hard to
maintain code when the attributes are undocumented because the user
has no way of knowing what will actually receive that attribute. They
also don't know which attributes are unsupported in this form (like
late parsed attributes, or ones with only a __declspec spelling, etc),
but that seems like less of a concern as we err if the attribute is
not supported.

~Aaron
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a subscriber: erik.pilkington.
dexonsmith added a comment.

In https://reviews.llvm.org/D30009#1218647, @rsmith wrote:

> One consequence of this patch (see https://reviews.llvm.org/rL341002) is that 
> adding documentation to an existing attribute //changes the behavior of 
> Clang//. That does not seem acceptable to me: documentation improvements 
> should not change which attributes we allow this pragma to appertain to.
>
> If we really want an "only documented attribtues can use this feature" rule 
> (and I'm really not convinced that is a useful rule to enforce


I also don't quite see why this is useful.

> ), I think the best way to do that is to add another flag on the Attr 
> instance in the .td file to opt the attribute out of `#pragma clang` support, 
> opt out all the undocumented attributes, and add an error to TableGen if we 
> find that an undocumented attribute has `#pragma clang` support enabled.

It seems arbitrary not to support an attribute from `#pragma clang attribute` 
just because it was once undocumented.  It does seem incrementally better, but 
still not great.

(FYI, @arphaman is on vacation until next week so he's unlikely to respond 
promptly.)


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In https://reviews.llvm.org/D30009#1218647, @rsmith wrote:

> One consequence of this patch (see https://reviews.llvm.org/rL341002) is that 
> adding documentation to an existing attribute //changes the behavior of 
> Clang//. That does not seem acceptable to me: documentation improvements 
> should not change which attributes we allow this pragma to appertain to.
>
> If we really want an "only documented attribtues can use this feature" rule 
> (and I'm really not convinced that is a useful rule to enforce), I think the 
> best way to do that is to add another flag on the Attr instance in the .td 
> file to opt the attribute out of `#pragma clang` support, opt out all the 
> undocumented attributes, and add an error to TableGen if we find that an 
> undocumented attribute has `#pragma clang` support enabled.


I've changed how we determine whether `#pragma clang attribute` is usable in 
https://reviews.llvm.org/rL341009 so that future documentation changes don't 
change clang's behavior. But we should still consider whether the attributes 
that were explicitly opted out of this feature in that revision really should 
be. What harm does allowing this feature for attributes that we don't document 
do? Most of those attributes are documented by GCC anyway, and the user 
experience of opting them out seems inconsistent and arbitrary. If the goal is 
to use this pragma as a forcing function to get documentation written for the 
attributes, it hasn't worked.


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.
Herald added a subscriber: llvm-commits.

One consequence of this patch (see https://reviews.llvm.org/rL341002) is that 
adding documentation to an existing attribute //changes the behavior of 
Clang//. That does not seem acceptable to me: documentation improvements should 
not change which attributes we allow this pragma to appertain to.

If we really want an "only documented attribtues can use this feature" rule 
(and I'm really not convinced that is a useful rule to enforce), I think the 
best way to do that is to add another flag on the Attr instance in the .td file 
to opt the attribute out of `#pragma clang` support, opt out all the 
undocumented attributes, and add an error to TableGen if we find that an 
undocumented attribute has `#pragma clang` support enabled.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

This LGTM, but you should give @rsmith some time to review before committing in 
case he catches something I've missed.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: docs/LanguageExtensions.rst:2380
+
+The ``__declspec`` style syntax is also supported. A single push directive
+accepts only one attribute regardless of the syntax used.

Might as well add an example for __declspec (since the other two have it), and 
move the restriction about the number of attributes supplied to its own 
paragraph for greater clarity.



Comment at: include/clang/Basic/DiagnosticGroups.td:462
 def IgnoredPragmas : DiagGroup<"ignored-pragmas", [IgnoredPragmaIntrinsic]>;
 def Pragmas : DiagGroup<"pragmas", [UnknownPragmas, IgnoredPragmas]>;
+def PragmaClangAttribute : DiagGroup<"pragma-clang-attribute">;

Should `PragmaClangAttribute` be added here as well?


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

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

[Duncan P. N. Exon Smith via cfe-commits]

> On Apr 5, 2017, at 05:44, Alex L  wrote:
> 
> 
> 
> On 5 April 2017 at 13:38, Duncan Exon Smith  > wrote:
> 
> 
> > On Apr 5, 2017, at 05:13, Aaron Ballman via Phabricator 
> > > wrote:
> >
> > aaron.ballman added inline comments.
> >
> >
> > 
> > Comment at: lib/Sema/SemaAttr.cpp:578
> > +return;
> > +  Diag(PragmaAttributeStack.back().Loc, 
> > diag::warn_pragm_attribute_no_pop_eof);
> > +}
> > 
> > arphaman wrote:
> >> aaron.ballman wrote:
> >>> Perhaps adding a FixIt here would be a kindness?
> >> Where would the fix-it point to? I think only the user will know the 
> >> location at which they meant to insert `#pragma clang attribute pop`.
> > Given that it's a warning rather than an error, and our recovery mechanism 
> > is to effectively pop the pragma at the end of the TU, I was thinking it 
> > could be added at the end of the TU. However, you are correct that it's 
> > probably not where the programmer needs it,
> 
> What about at the end of the file the push is in?  This is likely to be used 
> in header files, and it's probably unintentional if it extends past the end 
> of the file of the push.
> 
> Then we'd have to take into account preprocessor directives, as we'd 
> obviously want to insert it before the '#endif'. Or if the user used `#pragma 
> once`  then the end of the file could probably work.
> IMHO the improvement can be done as a follow-up that improves this and other 
> pragmas like #pragma pack.
> 

SGTM.

> 
> I think we should consider the same thing for #pragma pack (although that's a 
> little off-topic).
> 
> > so a FixIt likely isn't appropriate. Does that suggest the warning should 
> > be an error instead?
> 
> Maybe it should be an error, but I still think a fixit would be nice if we 
> can find a spot.
> 
> >
> >
> > 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

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

[Alex L via cfe-commits]

On 5 April 2017 at 13:38, Duncan Exon Smith  wrote:

>
>
> > On Apr 5, 2017, at 05:13, Aaron Ballman via Phabricator <
> revi...@reviews.llvm.org> wrote:
> >
> > aaron.ballman added inline comments.
> >
> >
> > 
> > Comment at: lib/Sema/SemaAttr.cpp:578
> > +return;
> > +  Diag(PragmaAttributeStack.back().Loc, diag::warn_pragm_attribute_no_
> pop_eof);
> > +}
> > 
> > arphaman wrote:
> >> aaron.ballman wrote:
> >>> Perhaps adding a FixIt here would be a kindness?
> >> Where would the fix-it point to? I think only the user will know the
> location at which they meant to insert `#pragma clang attribute pop`.
> > Given that it's a warning rather than an error, and our recovery
> mechanism is to effectively pop the pragma at the end of the TU, I was
> thinking it could be added at the end of the TU. However, you are correct
> that it's probably not where the programmer needs it,
>
> What about at the end of the file the push is in?  This is likely to be
> used in header files, and it's probably unintentional if it extends past
> the end of the file of the push.
>

Then we'd have to take into account preprocessor directives, as we'd
obviously want to insert it before the '#endif'. Or if the user used
`#pragma once`  then the end of the file could probably work.
IMHO the improvement can be done as a follow-up that improves this and
other pragmas like #pragma pack.


> I think we should consider the same thing for #pragma pack (although
> that's a little off-topic).
>
> > so a FixIt likely isn't appropriate. Does that suggest the warning
> should be an error instead?
>
> Maybe it should be an error, but I still think a fixit would be nice if we
> can find a spot.
>
> >
> >
> > 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

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

[Duncan Exon Smith via cfe-commits]

> On Apr 5, 2017, at 05:13, Aaron Ballman via Phabricator 
>  wrote:
> 
> aaron.ballman added inline comments.
> 
> 
> 
> Comment at: lib/Sema/SemaAttr.cpp:578
> +return;
> +  Diag(PragmaAttributeStack.back().Loc, 
> diag::warn_pragm_attribute_no_pop_eof);
> +}
> 
> arphaman wrote:
>> aaron.ballman wrote:
>>> Perhaps adding a FixIt here would be a kindness?
>> Where would the fix-it point to? I think only the user will know the 
>> location at which they meant to insert `#pragma clang attribute pop`.
> Given that it's a warning rather than an error, and our recovery mechanism is 
> to effectively pop the pragma at the end of the TU, I was thinking it could 
> be added at the end of the TU. However, you are correct that it's probably 
> not where the programmer needs it,

What about at the end of the file the push is in?  This is likely to be used in 
header files, and it's probably unintentional if it extends past the end of the 
file of the push. 

I think we should consider the same thing for #pragma pack (although that's a 
little off-topic).

> so a FixIt likely isn't appropriate. Does that suggest the warning should be 
> an error instead?

Maybe it should be an error, but I still think a fixit would be nice if we can 
find a spot. 

> 
> 
> 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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: lib/Sema/SemaAttr.cpp:578
+return;
+  Diag(PragmaAttributeStack.back().Loc, diag::warn_pragm_attribute_no_pop_eof);
+}

arphaman wrote:
> aaron.ballman wrote:
> > Perhaps adding a FixIt here would be a kindness?
> Where would the fix-it point to? I think only the user will know the location 
> at which they meant to insert `#pragma clang attribute pop`.
Given that it's a warning rather than an error, and our recovery mechanism is 
to effectively pop the pragma at the end of the TU, I was thinking it could be 
added at the end of the TU. However, you are correct that it's probably not 
where the programmer needs it, so a FixIt likely isn't appropriate. Does that 
suggest the warning should be an error instead?


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

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: lib/Sema/SemaAttr.cpp:578
+return;
+  Diag(PragmaAttributeStack.back().Loc, diag::warn_pragm_attribute_no_pop_eof);
+}

aaron.ballman wrote:
> Perhaps adding a FixIt here would be a kindness?
Where would the fix-it point to? I think only the user will know the location 
at which they meant to insert `#pragma clang attribute pop`.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a reviewer: rsmith.
aaron.ballman added a comment.

Adding Richard, since this is a fairly extensive change to the frontend and he 
may have some opinions as well.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for the update! I think this is getting close, though I have a few 
more comments inline.




Comment at: docs/LanguageExtensions.rst:2338
+The attributes can also be written using the C++11 style syntax, as long
+as only one attribute is specified in the square brackets:
+

Do we want to have the same restriction for GNU and declspec attributes, that 
only a single one can appear? If so, we should mention it more generally.



Comment at: docs/LanguageExtensions.rst:2420
+- ``variable``: Can be used to apply attributes to variables, including
+  local variables, parameters, and global variables.
+

Are member variables included in this list? Perhaps this should also mention 
that it does not apply to Objective-C ivars.



Comment at: include/clang/Basic/AttrSubjectMatchRules.h:17
+namespace clang {
+
+namespace attr {

Can remove the newline.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:765
+  "'#pragma clang attribute push' regions ends here">;
+def warn_pragm_attribute_no_pop_eof : Warning<"unterminated "
+  "'#pragma clang attribute push' at end of file">,

Typo: pragm

Is there value in this being in its own warning group from 
pragma-attribute-unused? It might make sense to disable any pragma attribute 
warning under the same group name, but I don't feel super strongly about it.



Comment at: include/clang/Sema/Sema.h:8160
+  /// '\#pragma clang attribute push' directives to the given declaration.
+  void AddPragmaAttributes(Scope *S, Decl *D);
+

arphaman wrote:
> aaron.ballman wrote:
> > I worry about new calls to ProcessDeclAttributes() that don't have a nearby 
> > call to AddPragmaAttributes(). I wonder if there's a way that we can combat 
> > that likely misuse?
> Perhaps we could just always call `AddPragmaAttributes` from 
> `ProcessDeclAttributes` (after the declarator attributes are applied). That 
> would mean that `#pragma clang attribute` will apply its attributes earlier 
> than other pragmas or implicit attributes.
I was wondering whether that would be a reasonable approach. I *think* it seems 
reasonable, however.



Comment at: include/clang/Sema/Sema.h:8160
+  /// '\#pragma clang attribute push' directives to the given declaration.
+  void AddPragmaAttributes(Scope *S, Decl *D);
+

aaron.ballman wrote:
> arphaman wrote:
> > aaron.ballman wrote:
> > > I worry about new calls to ProcessDeclAttributes() that don't have a 
> > > nearby call to AddPragmaAttributes(). I wonder if there's a way that we 
> > > can combat that likely misuse?
> > Perhaps we could just always call `AddPragmaAttributes` from 
> > `ProcessDeclAttributes` (after the declarator attributes are applied). That 
> > would mean that `#pragma clang attribute` will apply its attributes earlier 
> > than other pragmas or implicit attributes.
> I was wondering whether that would be a reasonable approach. I *think* it 
> seems reasonable, however.
Phab won't let me edit my comment for some reason, so replying a second time. I 
don't think it's a problem for the attributes to be added earlier because 
attributes are essentially an unordered list on the declarations anyways. If 
this really is a problem, I would hope that there's a test that will suddenly 
start failing for us.



Comment at: lib/Sema/SemaAttr.cpp:631
+ProcessDeclAttributeList(S, D, Entry.Attribute);
+HasFailed = Trap.hasErrorOccurred();
+  }

arphaman wrote:
> aaron.ballman wrote:
> > Do we only care about errors, or should we also care about warnings? 
> > Incorrect attributes that do not affect code generation often are treated 
> > as warnings (and the attribute is simply ignored) rather than errors, so 
> > the user may want to know about those issues.
> I think it's important to try and get the warnings right as well. I think it 
> would make sense to emit the warnings for each declaration that receives the 
> attribute (including the first one). One issue that I found was that warnings 
> didn't tell the user which declaration was receiving the attribute. The 
> updated patch fixes that by ensuring that warnings are emitted for all 
> receivers and that each warning has an additional note that points to the 
> declaration that receives the attribute.
> 
> This made me also rethink the SFINAE trap completely. I decided to avoid it. 
> Originally I thought that it would be nice to have it so that we can avoid 
> duplicate attribute-related errors when applying an attribute to each 
> declaration.  There were a couple of issues with that approach:
> - Attributes have receiver dependent errors/warnings that should be reported 
> for each declaration. Previously the patch was inconsistent for such errors; 
> it stopped applying the attribute only when the first 

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: docs/LanguageExtensions.rst:2430
+
+- ``function(is_method)``: Can be used to apply attributes to C++ methods,
+  including operators and constructors/destructors.

aaron.ballman wrote:
> Instead of `is_method`, I think we should use `is_instance`, `is_member`, or 
> something else. C++ doesn't really use the terminology "method", but 
> Objective-C does, which is why this could be confusing. It might also be good 
> to mention the behavior of a static member functions.
> 
> Whatever terminology gets picked, you should use that phrasing wherever you 
> say "C++ method".
I think `function(is_member)` should work well.



Comment at: docs/LanguageExtensions.rst:2443
+
+- ``record(unless(is_union))``: Can be used to apply attributes only to
+  ``struct`` and ``class`` declarations.

aaron.ballman wrote:
> Does unless() work in other combinations? e.g., can I use 
> record(unless(is_enum))? I'm not saying that it has to, but it would be good 
> to document one way or the other.
No, right now `unless` is very rigid and only works with a select set of match 
sub rules which are needed for attributes. I have noted that in the docs.



Comment at: docs/LanguageExtensions.rst:2448
+
+- ``enum_case``: Can be used to apply attributes to enumerators.
+

aaron.ballman wrote:
> I'd prefer these to be named `enumeration` and `enumerator`, `enum` and 
> `enum_constant`, or something. `enum_case` is too novel of a term for me.
I think `enum_constant` would work. I was thinking about `enumeration` and 
`enumerator` before, but they just look too alike so it might be easy to 
confuse them.



Comment at: docs/LanguageExtensions.rst:2486
+
+- ``block``: Can be used to apply attributes to block declarations. This does
+  not include variables/fields of block pointer type.

aaron.ballman wrote:
> Perhaps name this one `objc_block` to be consistent with the other 
> objective-c features?
Blocks can be used outside of Objective-C.



Comment at: docs/LanguageExtensions.rst:2497
+:doc:`individual documentation for that attribute `.
+
+The attributes are applied to a declaration only when there would

aaron.ballman wrote:
> We should also support __declspec attributes shouldn't we (I see no reason to 
> support 2 out of the 3 attribute styles)? If so, the docs should state that, 
> and if not, I'd like to understand why and the docs should still state that.
Sure. A proper consistent style with `__attribute__` as you mentioned earlier 
is better for us as well due to legacy macros. Now `__attribute__` is required 
and I also added support for the `__declspec` attributes.



Comment at: include/clang/Sema/Sema.h:8160
+  /// '\#pragma clang attribute push' directives to the given declaration.
+  void AddPragmaAttributes(Scope *S, Decl *D);
+

aaron.ballman wrote:
> I worry about new calls to ProcessDeclAttributes() that don't have a nearby 
> call to AddPragmaAttributes(). I wonder if there's a way that we can combat 
> that likely misuse?
Perhaps we could just always call `AddPragmaAttributes` from 
`ProcessDeclAttributes` (after the declarator attributes are applied). That 
would mean that `#pragma clang attribute` will apply its attributes earlier 
than other pragmas or implicit attributes.



Comment at: lib/Parse/ParsePragma.cpp:1158
+  Diag(Loc, diag::err_pragma_attribute_multiple_attributes);
+  Attrs.getList()->setNext(nullptr);
+}

aaron.ballman wrote:
> Is this going to cause a memory leak for those attributes that were parsed 
> but dropped?
Possibly, I fixed this up.



Comment at: lib/Sema/SemaAttr.cpp:602-603
+  PragmaAttributeStack.pop_back();
+  // FIXME: Should we check the attribute even when it's not applied to any
+  // declaration?
+}

aaron.ballman wrote:
> I think that makes sense (I can imagine someone writing the pragma push and 
> pop before writing all their declarations), but I might be misunderstanding 
> the question.
That would make sense, I agree. I added a warning that warns about unapplied 
attributes in push pop regions. However, my original question was more about 
correctness of the attribute which we can’t check semantically unless we apply 
it first. I don’t think it’s really needed though, so I removed the “fixme”.



Comment at: lib/Sema/SemaAttr.cpp:631
+ProcessDeclAttributeList(S, D, Entry.Attribute);
+HasFailed = Trap.hasErrorOccurred();
+  }

aaron.ballman wrote:
> Do we only care about errors, or should we also care about warnings? 
> Incorrect attributes that do not affect code generation often are treated as 
> warnings (and the attribute is simply ignored) rather than errors, so the 
> user may want to know about those 

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D30009#706515, @aaron.ballman wrote:

> In https://reviews.llvm.org/D30009#706171, @arphaman wrote:
>
> > I would be ok with that. We could merge `apply_to` and `apply_only_to` into 
> > a single `apply_to` matching rule set specifier (it would behave like 
> > `apply_only_to`).
>
>
> That sounds sensible to me.
>
> > I guess one downside would be is that it will become harder to fill out all 
> > the match rules if one wants to apply an attribute to all possible 
> > declarations. I suppose the attribute documentation generator can be 
> > updated to include the full match rule set for each attribute instead of 
> > just yes/no in the `#pragma clang attribute` documentation column.
>
> Okay, so here's a possibly crazy idea (and it may be way too magical): what 
> if `#pragma clang attribute push(foo)` generated the fix-it hint to suggest 
> all of the targets the attribute can apply to? Alternatively, what if any 
> malformed parsing of `#pragma clang attribute push(foo)` automatically do 
> this? We know the user is trying to apply attributes to declarations, and we 
> know which attribute they're trying for, so it somewhat stands to reason that 
> the rest of the syntax can be supplied for them... and we need *something* in 
> apply_to, so why not default to everything?


Something like that should work. Although we probably still want to have them 
in the documentation as well.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D30009#706171, @arphaman wrote:

> I would be ok with that. We could merge `apply_to` and `apply_only_to` into a 
> single `apply_to` matching rule set specifier (it would behave like 
> `apply_only_to`).


That sounds sensible to me.

> I guess one downside would be is that it will become harder to fill out all 
> the match rules if one wants to apply an attribute to all possible 
> declarations. I suppose the attribute documentation generator can be updated 
> to include the full match rule set for each attribute instead of just yes/no 
> in the `#pragma clang attribute` documentation column.

Okay, so here's a possibly crazy idea (and it may be way too magical): what if 
`#pragma clang attribute push(foo)` generated the fix-it hint to suggest all of 
the targets the attribute can apply to? Alternatively, what if any malformed 
parsing of `#pragma clang attribute push(foo)` automatically do this? We know 
the user is trying to apply attributes to declarations, and we know which 
attribute they're trying for, so it somewhat stands to reason that the rest of 
the syntax can be supplied for them... and we need *something* in apply_to, so 
why not default to everything?


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

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D30009#705699, @aaron.ballman wrote:

> In https://reviews.llvm.org/D30009#705649, @efriedma wrote:
>
> > Looking over the most recent version, I'm happy with the general semantics 
> > of push with apply_only_to.  I'm not sure I see the point of apply_to: it 
> > doesn't allow the user to do anything that can't be done with 
> > apply_only_to.  Also, if the apply_to list for an attribute ever changes, 
> > it becomes impossible to write code which supports both versions of the 
> > compiler.
>


This is a good point that I haven't considered.

> You bring up a really good point about compiler versioning -- it would be 
> pretty awful to force the user to use #ifs to deal with that.
> 
> I believe apply_to is somewhat useful so that the user knows what an 
> attribute actually appertains to, and get a diagnostic if we ever extend the 
> list of things their particular attribute can appertain to so they are forced 
> to decide whether they want that new behavior or not.
> 
> @arphaman, what do you think about the idea of only having apply_to with the 
> same semantics as you currently have for apply_only_to?

I would be ok with that. We could merge `apply_to` and `apply_only_to` into a 
single `apply_to` matching rule set specifier (it would behave like 
`apply_only_to`).

I guess one downside would be is that it will become harder to fill out all the 
match rules if one wants to apply an attribute to all possible declarations. I 
suppose the attribute documentation generator can be updated to include the 
full match rule set for each attribute instead of just yes/no in the `#pragma 
clang attribute` documentation column.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D30009#705649, @efriedma wrote:

> Looking over the most recent version, I'm happy with the general semantics of 
> push with apply_only_to.  I'm not sure I see the point of apply_to: it 
> doesn't allow the user to do anything that can't be done with apply_only_to.  
> Also, if the apply_to list for an attribute ever changes, it becomes 
> impossible to write code which supports both versions of the compiler.


You bring up a really good point about compiler versioning -- it would be 
pretty awful to force the user to use #ifs to deal with that.

I believe apply_to is somewhat useful so that the user knows what an attribute 
actually appertains to, and get a diagnostic if we ever extend the list of 
things their particular attribute can appertain to so they are forced to decide 
whether they want that new behavior or not.

@arphaman, what do you think about the idea of only having apply_to with the 
same semantics as you currently have for 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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

Looking over the most recent version, I'm happy with the general semantics of 
push with apply_only_to.  I'm not sure I see the point of apply_to: it doesn't 
allow the user to do anything that can't be done with apply_only_to.  Also, if 
the apply_to list for an attribute ever changes, it becomes impossible to write 
code which supports both versions of the compiler.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I really like the way this feature is shaping up! I apologize for how long it 
took to review the code (I was out for work for two weeks and this is a pretty 
extensive patch). Thank you for the continued efforts on this.




Comment at: docs/LanguageExtensions.rst:2323
+follow the pragma receive the attributes that are on the attribute stack, until
+the stack is cleared using a ``#pragma clang attribute pop`` directive.
+

Might want to mention explicitly that these directives nest.



Comment at: docs/LanguageExtensions.rst:2337
+The attributes can also be written using the C++11 style syntax, as long
+as just one attribute is specified in the square brackets:
+

just -> only



Comment at: docs/LanguageExtensions.rst:2352
+depends on the subject match rules that were specified in the pragma. Subject
+match rules are specified after the attribute. Firstly, the compiler expects an
+identifier that corresponds to the subject set specifier. The ``apply_to``

Drop the "Firstly, ".



Comment at: docs/LanguageExtensions.rst:2356
+**strict** subject set. Strict subject sets have to be identical to the
+attribute's own subject set, otherwise a compilation error is presented. For
+example, an attribute like ``[[noreturn]]`` whose subject set is just

"otherwise it results in an error." ?



Comment at: docs/LanguageExtensions.rst:2378
+  // This is an error as well, [[noreturn]] must be applied *only* to function:
+  #pragma clang attribute push([[noreturn]], apply_to = any(function, 
variable))
+

We don't describe `any` below, but should.



Comment at: docs/LanguageExtensions.rst:2387
+The ``apply_to`` specifier should be used whenever one wants to apply an
+attribute to all of the declaration that can receive that attribute. The use
+of ``apply_to`` ensures that future versions of compiler will report an error

declarations (plural).



Comment at: docs/LanguageExtensions.rst:2388
+attribute to all of the declaration that can receive that attribute. The use
+of ``apply_to`` ensures that future versions of compiler will report an error
+if the attribute will start supporting a new kind of declaration.

of compiler -> of the compiler



Comment at: docs/LanguageExtensions.rst:2389
+of ``apply_to`` ensures that future versions of compiler will report an error
+if the attribute will start supporting a new kind of declaration.
+

will start supporting -> supports



Comment at: docs/LanguageExtensions.rst:2430
+
+- ``function(is_method)``: Can be used to apply attributes to C++ methods,
+  including operators and constructors/destructors.

Instead of `is_method`, I think we should use `is_instance`, `is_member`, or 
something else. C++ doesn't really use the terminology "method", but 
Objective-C does, which is why this could be confusing. It might also be good 
to mention the behavior of a static member functions.

Whatever terminology gets picked, you should use that phrasing wherever you say 
"C++ method".



Comment at: docs/LanguageExtensions.rst:2435
+  methods, and variables/fields whose type is a function pointer. It does not
+  apply attributes to Objective-C methods or blocks!
+

Replace the exclamation point with a full stop.



Comment at: docs/LanguageExtensions.rst:2440
+
+- ``record``: Can be used to apply attributes to ``struct``, ``class`` and
+  ``union`` declarations.

Insert a comma after class.



Comment at: docs/LanguageExtensions.rst:2443
+
+- ``record(unless(is_union))``: Can be used to apply attributes only to
+  ``struct`` and ``class`` declarations.

Does unless() work in other combinations? e.g., can I use 
record(unless(is_enum))? I'm not saying that it has to, but it would be good to 
document one way or the other.



Comment at: docs/LanguageExtensions.rst:2448
+
+- ``enum_case``: Can be used to apply attributes to enumerators.
+

I'd prefer these to be named `enumeration` and `enumerator`, `enum` and 
`enum_constant`, or something. `enum_case` is too novel of a term for me.



Comment at: docs/LanguageExtensions.rst:2451
+- ``variable``: Can be used to apply attributes to variables, including
+  local variables, parameters and global variables.
+

Comma after parameters.



Comment at: docs/LanguageExtensions.rst:2462
+
+- ``variable(unless(is_parameter))``: Can be used to apply attributes to all
+  the variables that are not parameters.

Same question here as above with the unless(), can I specify any is_foo there?



Comment at: 

[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:
> > > 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 

[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:
> > 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. 

[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, 

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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:
> 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.


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

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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: docs/LanguageExtensions.rst:2349
+attribute is supported by the pragma by referring to the
+:doc:`individual documentation for that attribute `.

arphaman wrote:
> efriedma wrote:
> > arphaman wrote:
> > > arphaman wrote:
> > > > efriedma wrote:
> > > > > I'm wondering if we can tweak the approach so that we don't have to 
> > > > > separately define how this works for each attribute; for example, 
> > > > > `#pragma clang attribute_function_declaration push(...)` would apply 
> > > > > to each function declaration, `#pragma clang 
> > > > > attribute_global_variable_declaration push(...)` would apply to each 
> > > > > global variable declaration, etc.
> > > > I agree with this idea, I think it would be useful to have the ability 
> > > > to specify the target declarations. I do think it would be better to 
> > > > use the 'clang attribute' umbrella pragma, and maybe add an extra 
> > > > argument to the 'push', e.g.:
> > > > 
> > > > ```
> > > > #pragma attribute push (annotate("functions-only"), 
> > > > applicable_to=function) // or maybe received_by=?
> > > > #pragma attribute push (annotate("struct+enum"), applicable_to=struct, 
> > > > applicable_to=enum)
> > > > ```
> > > I think that the further tweaks that control which declarations receive 
> > > the attributes should be kept for a follow-up patch.
> > I'm not sure we can wait to address this, especially if we're turning this 
> > on for all attributes by default.  There's a compatibility hazard here: if 
> > the push doesn't specify what declarations it applies to, we can never 
> > change AttributeAppliesToDecl for any existing attribute.
> Are you saying that we should never allow a push without specifying which 
> declarations should the attribute apply to? I think that would make this 
> feature less useful, as some attributes have a well defined set of 
> declarations that they apply to. Even if we will change the attribute's 
> subjects in the future, I think it might be more likely that the users 
> would've wanted to apply the attribute to the updated compiler-determined 
> subject list.
I agree, users shouldn't have to repeat that attribute "target" applies to 
functions and attribute "warn_unused" applies to record types. Asking the user 
to figure it out just makes it harder to use.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: docs/LanguageExtensions.rst:2418
+In general, the attributes are applied to a declaration only when there would
+have been no error or warning for that attribute if it was specified 
explicitly.
+An attribute is applied to each relevant declaration individually,

So then this code does not produce an error?
```
#pragma clang attribute push(hot)

void func(void) __attribute__((cold));

#pragma clang attribute pop
```



Comment at: docs/LanguageExtensions.rst:2428
+whether or not an attribute is supported by the pragma by referring to the
+:doc:`individual documentation for that attribute `.

This doesn't make clear what would happen with something like:
```
  #pragma clang attribute push(cdecl)

  void function(void (*fp)(int));

  #pragma clang attribute pop
```
Naively, I would expect the cdecl attribute to appertain to both `function` and 
`fp`, but I'm guessing that's not what actually happens (because cdecl is both 
a decl and a type attribute at the same time).

Also, why is this not available for __declspec attributes?

Why do C++ attributes require the syntax to be present in the #pragma, but 
GNU-style attributes do not? There are also MS-style attribute (currently 
unsupported, but there have been some efforts to include them in the past) and 
__declspec attributes as well. Why not require the syntax so that any of them 
can be supported?



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

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.


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

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: docs/LanguageExtensions.rst:2349
+attribute is supported by the pragma by referring to the
+:doc:`individual documentation for that attribute `.

efriedma wrote:
> arphaman wrote:
> > arphaman wrote:
> > > efriedma wrote:
> > > > I'm wondering if we can tweak the approach so that we don't have to 
> > > > separately define how this works for each attribute; for example, 
> > > > `#pragma clang attribute_function_declaration push(...)` would apply to 
> > > > each function declaration, `#pragma clang 
> > > > attribute_global_variable_declaration push(...)` would apply to each 
> > > > global variable declaration, etc.
> > > I agree with this idea, I think it would be useful to have the ability to 
> > > specify the target declarations. I do think it would be better to use the 
> > > 'clang attribute' umbrella pragma, and maybe add an extra argument to the 
> > > 'push', e.g.:
> > > 
> > > ```
> > > #pragma attribute push (annotate("functions-only"), 
> > > applicable_to=function) // or maybe received_by=?
> > > #pragma attribute push (annotate("struct+enum"), applicable_to=struct, 
> > > applicable_to=enum)
> > > ```
> > I think that the further tweaks that control which declarations receive the 
> > attributes should be kept for a follow-up patch.
> I'm not sure we can wait to address this, especially if we're turning this on 
> for all attributes by default.  There's a compatibility hazard here: if the 
> push doesn't specify what declarations it applies to, we can never change 
> AttributeAppliesToDecl for any existing attribute.
Are you saying that we should never allow a push without specifying which 
declarations should the attribute apply to? I think that would make this 
feature less useful, as some attributes have a well defined set of declarations 
that they apply to. Even if we will change the attribute's subjects in the 
future, I think it might be more likely that the users would've wanted to apply 
the attribute to the updated compiler-determined subject list.


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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: docs/LanguageExtensions.rst:2349
+attribute is supported by the pragma by referring to the
+:doc:`individual documentation for that attribute `.

arphaman wrote:
> arphaman wrote:
> > efriedma wrote:
> > > I'm wondering if we can tweak the approach so that we don't have to 
> > > separately define how this works for each attribute; for example, 
> > > `#pragma clang attribute_function_declaration push(...)` would apply to 
> > > each function declaration, `#pragma clang 
> > > attribute_global_variable_declaration push(...)` would apply to each 
> > > global variable declaration, etc.
> > I agree with this idea, I think it would be useful to have the ability to 
> > specify the target declarations. I do think it would be better to use the 
> > 'clang attribute' umbrella pragma, and maybe add an extra argument to the 
> > 'push', e.g.:
> > 
> > ```
> > #pragma attribute push (annotate("functions-only"), applicable_to=function) 
> > // or maybe received_by=?
> > #pragma attribute push (annotate("struct+enum"), applicable_to=struct, 
> > applicable_to=enum)
> > ```
> I think that the further tweaks that control which declarations receive the 
> attributes should be kept for a follow-up patch.
I'm not sure we can wait to address this, especially if we're turning this on 
for all attributes by default.  There's a compatibility hazard here: if the 
push doesn't specify what declarations it applies to, we can never change 
AttributeAppliesToDecl for any existing attribute.


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

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: docs/LanguageExtensions.rst:2349
+attribute is supported by the pragma by referring to the
+:doc:`individual documentation for that attribute `.

arphaman wrote:
> efriedma wrote:
> > I'm wondering if we can tweak the approach so that we don't have to 
> > separately define how this works for each attribute; for example, `#pragma 
> > clang attribute_function_declaration push(...)` would apply to each 
> > function declaration, `#pragma clang attribute_global_variable_declaration 
> > push(...)` would apply to each global variable declaration, etc.
> I agree with this idea, I think it would be useful to have the ability to 
> specify the target declarations. I do think it would be better to use the 
> 'clang attribute' umbrella pragma, and maybe add an extra argument to the 
> 'push', e.g.:
> 
> ```
> #pragma attribute push (annotate("functions-only"), applicable_to=function) 
> // or maybe received_by=?
> #pragma attribute push (annotate("struct+enum"), applicable_to=struct, 
> applicable_to=enum)
> ```
I think that the further tweaks that control which declarations receive the 
attributes should be kept for a follow-up patch.


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

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 88764.
arphaman marked an inline comment as done.
arphaman added a comment.

The updated patch switches over to the opt-out approach, allows the C++11 style 
syntax and improves documentation.


Repository:
  rL LLVM

https://reviews.llvm.org/D30009

Files:
  docs/LanguageExtensions.rst
  include/clang/Basic/Attr.td
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/TokenKinds.def
  include/clang/Parse/Parser.h
  include/clang/Sema/AttributeList.h
  include/clang/Sema/Sema.h
  lib/Parse/ParsePragma.cpp
  lib/Parse/ParseStmt.cpp
  lib/Parse/Parser.cpp
  lib/Sema/AttributeList.cpp
  lib/Sema/SemaAttr.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaDeclObjC.cpp
  lib/Sema/SemaObjCProperty.cpp
  test/Parser/pragma-attribute.cpp
  test/Sema/pragma-attribute.c
  test/SemaCXX/pragma-attribute.cpp
  test/SemaObjC/pragma-attribute.m
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -1522,6 +1522,38 @@
   OS << "#endif // CLANG_ATTR_LATE_PARSED_LIST\n\n";
 }
 
+static bool hasGNUorCXX11Spelling(const Record ) {
+  std::vector Spellings = GetFlattenedSpellings(Attribute);
+  for (const auto  : Spellings) {
+if (I.variety() == "GNU" || I.variety() == "CXX11")
+  return true;
+  }
+  return false;
+}
+
+static bool isSupportedByPragmaAttribute(const Record ) {
+  if (Attribute.getValueAsBit("ForcePragmaAttributeSupport"))
+return true;
+  // Opt-out rules:
+  // An attribute requires delayed parsing (LateParsed is on)
+  if (Attribute.getValueAsBit("LateParsed"))
+return false;
+  // An attribute has no GNU/CXX11 spelling
+  if (!hasGNUorCXX11Spelling(Attribute))
+return false;
+  // An attribute has no subject list or its subject list is empty
+  if (Attribute.isValueUnset("Subjects"))
+return false;
+  const Record *SubjectObj = Attribute.getValueAsDef("Subjects");
+  std::vector Subjects = SubjectObj->getValueAsListOfDefs("Subjects");
+  if (Subjects.empty())
+return false;
+  // An attribute derives from a StmtAttr or a TypeAttr
+  if (Attribute.isSubClassOf("TypeAttr") || Attribute.isSubClassOf("StmtAttr"))
+return false;
+  return true;
+}
+
 template 
 static void forEachUniqueSpelling(const Record , Fn &) {
   std::vector Spellings = GetFlattenedSpellings(Attr);
@@ -2692,9 +2724,13 @@
   return B + "Decl";
 }
 
+static std::string functionNameForCustomAppertainsTo(const Record ) {
+  return "is" + Subject.getName().str();
+}
+
 static std::string GenerateCustomAppertainsTo(const Record ,
   raw_ostream ) {
-  std::string FnName = "is" + Subject.getName().str();
+  std::string FnName = functionNameForCustomAppertainsTo(Subject);
 
   // If this code has already been generated, simply return the previous
   // instance of it.
@@ -2779,6 +2815,50 @@
   return FnName;
 }
 
+static void GenerateDefaultAppliesTo(raw_ostream ) {
+  OS << "static bool defaultAttributeAppliesTo(Sema &, const Decl *) {\n";
+  OS << "  return true;\n";
+  OS << "}\n\n";
+}
+
+static std::string GenerateAppliesTo(const Record , raw_ostream ) {
+  // If the attribute doesn't support '#pragma clang attribute' or if it doesn't
+  // contain a subjects definition, then use the default appliedTo logic.
+  if (!isSupportedByPragmaAttribute(Attr) || Attr.isValueUnset("Subjects"))
+return "defaultAttributeAppliesTo";
+
+  const Record *SubjectObj = Attr.getValueAsDef("Subjects");
+  std::vector Subjects = SubjectObj->getValueAsListOfDefs("Subjects");
+
+  // If the list of subjects is empty, it is assumed that the attribute applies
+  // to everything.
+  if (Subjects.empty())
+return "defaultAttributeAppliesTo";
+
+  // Otherwise, generate an appliesTo check specific to this attribute which
+  // checks all of the given subjects against the Decl passed in. Return the
+  // name of that check to the caller.
+  std::string FnName = "check" + Attr.getName().str() + "AttributeAppliesTo";
+  std::stringstream SS;
+  SS << "static bool " << FnName << "(Sema , const Decl *D) {\n";
+  SS << "  return ";
+  for (auto I = Subjects.begin(), E = Subjects.end(); I != E; ++I) {
+// If the subject has custom code associated with it, use the function
+// that was generated for GenerateAppertainsTo to check if the declaration
+// is valid.
+if ((*I)->isSubClassOf("SubsetSubject"))
+  SS << functionNameForCustomAppertainsTo(**I) << "(D)";
+else
+  SS << "isa<" << GetSubjectWithSuffix(*I) << ">(D)";
+
+if (I + 1 != E)
+  SS << " || ";
+  }
+  SS << ";\n}\n\n";
+  OS << SS.str();
+  return FnName;
+}
+
 static void GenerateDefaultLangOptRequirements(raw_ostream ) {
   OS << "static bool defaultDiagnoseLangOpts(Sema &, ";
   OS << "const 

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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D30009#678890, @arphaman wrote:

> In https://reviews.llvm.org/D30009#678091, @hfinkel wrote:
>
> > I don't understand why it only supports some attributes. Is there some 
> > handling that needs to take place (I don't see anything obvious in this 
> > patch)? If most attributes will "just work", I'd much rather opt-out the 
> > few exceptions (which we can then explicitly document), if any, than using 
> > this opt-in solution.
>
>
> I think it would be reasonable to use an opt-out approach. I think the 
> following set of initial rules should determine when an attribute should not 
> be supported by the pragma:
>
> - If an attribute is late parsed
> - Or if it has no GNU/CXX11 spelling
> - Or if it has no subject list, or its subject list is empty (excluding 
> annotate)
> - Or if it derives from StmtAttr or TypeAttr


SGTM.


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

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

In https://reviews.llvm.org/D30009#678091, @hfinkel wrote:

> I don't understand why it only supports some attributes. Is there some 
> handling that needs to take place (I don't see anything obvious in this 
> patch)? If most attributes will "just work", I'd much rather opt-out the few 
> exceptions (which we can then explicitly document), if any, than using this 
> opt-in solution.


I think it would be reasonable to use an opt-out approach. I think the 
following set of initial rules should determine when an attribute should not be 
supported by the pragma:

- If an attribute is late parsed
- Or if it has no GNU/CXX11 spelling
- Or if it has no subject list, or its subject list is empty (excluding 
annotate)
- Or if it derives from StmtAttr or TypeAttr


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

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

[Richard Smith via cfe-commits]

On 15 February 2017 at 17:45, Hal Finkel via Phabricator via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> hfinkel added a comment.
>
> I don't understand why it only supports some attributes. Is there some
> handling that needs to take place (I don't see anything obvious in this
> patch)? If most attributes will "just work", I'd much rather opt-out the
> few exceptions (which we can then explicitly document), if any, than using
> this opt-in solution.


Only supporting attributes with a GNU spelling also seems a bit strange to
me.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Hal Finkel via Phabricator via cfe-commits]

hfinkel added a comment.

I don't understand why it only supports some attributes. Is there some handling 
that needs to take place (I don't see anything obvious in this patch)? If most 
attributes will "just work", I'd much rather opt-out the few exceptions (which 
we can then explicitly document), if any, than using this opt-in solution.


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

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added inline comments.



Comment at: docs/LanguageExtensions.rst:2349
+attribute is supported by the pragma by referring to the
+:doc:`individual documentation for that attribute `.

efriedma wrote:
> I'm wondering if we can tweak the approach so that we don't have to 
> separately define how this works for each attribute; for example, `#pragma 
> clang attribute_function_declaration push(...)` would apply to each function 
> declaration, `#pragma clang attribute_global_variable_declaration push(...)` 
> would apply to each global variable declaration, etc.
I agree with this idea, I think it would be useful to have the ability to 
specify the target declarations. I do think it would be better to use the 
'clang attribute' umbrella pragma, and maybe add an extra argument to the 
'push', e.g.:

```
#pragma attribute push (annotate("functions-only"), applicable_to=function) // 
or maybe received_by=?
#pragma attribute push (annotate("struct+enum"), applicable_to=struct, 
applicable_to=enum)
```


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

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

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: docs/LanguageExtensions.rst:2344
+declaration failed to receive the attribute because of a compilation error. The
+attributes that aren't applied to any declaration are not verified 
semantically.
+

I think "to each declaration individually" needs to expanded on a bit here.  
It's not clear how this interacts with namespaces, or classes, or function 
declarations, or function definitions, or templates.  For example, if you 
declare a function inside of a `#pragma clang attribute 
push(annotate("custom"))`, does that add an attribute to each parameter of the 
function?



Comment at: docs/LanguageExtensions.rst:2349
+attribute is supported by the pragma by referring to the
+:doc:`individual documentation for that attribute `.

I'm wondering if we can tweak the approach so that we don't have to separately 
define how this works for each attribute; for example, `#pragma clang 
attribute_function_declaration push(...)` would apply to each function 
declaration, `#pragma clang attribute_global_variable_declaration push(...)` 
would apply to each global variable declaration, etc.


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

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

[Alex Lorenz via Phabricator via cfe-commits]

arphaman created this revision.

This patch adds support for the `#pragma clang attribute` directive that was 
proposed recently at 
http://lists.llvm.org/pipermail/cfe-dev/2017-February/052689.html.

Initially it supports the `annotate`, `require_constant_initialization` and 
`objc_subclassing_restricted` attribute (I added support for the last two to 
verify that only those declarations that are specified in the Attr.td subject 
list can receive the attribute). The attributes are parsed immediately and are 
applied individually to each relevant declaration. The attribute application 
errors aren't reported more than once. The `annotate` attribute, which doesn't 
have the subject list, is applied only to those declarations that can receive 
explicit GNU-style attributes.

Thanks


Repository:
  rL LLVM

https://reviews.llvm.org/D30009

Files:
  docs/LanguageExtensions.rst
  include/clang/Basic/Attr.td
  include/clang/Basic/DiagnosticParseKinds.td
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Basic/TokenKinds.def
  include/clang/Parse/Parser.h
  include/clang/Sema/AttributeList.h
  include/clang/Sema/Sema.h
  lib/Parse/ParsePragma.cpp
  lib/Parse/ParseStmt.cpp
  lib/Parse/Parser.cpp
  lib/Sema/AttributeList.cpp
  lib/Sema/SemaAttr.cpp
  lib/Sema/SemaDecl.cpp
  lib/Sema/SemaDeclCXX.cpp
  lib/Sema/SemaDeclObjC.cpp
  lib/Sema/SemaObjCProperty.cpp
  test/Parser/pragma-attribute.cpp
  test/Sema/pragma-attribute.c
  test/SemaCXX/pragma-attribute.cpp
  test/SemaObjC/pragma-attribute.m
  utils/TableGen/ClangAttrEmitter.cpp

Index: utils/TableGen/ClangAttrEmitter.cpp
===
--- utils/TableGen/ClangAttrEmitter.cpp
+++ utils/TableGen/ClangAttrEmitter.cpp
@@ -1522,6 +1522,20 @@
   OS << "#endif // CLANG_ATTR_LATE_PARSED_LIST\n\n";
 }
 
+// Emits the SupportsPragmaAttribute property for attributes.
+static void emitClangAttrPragmaAttributeSupportList(RecordKeeper ,
+raw_ostream ) {
+  OS << "#if defined(CLANG_ATTR_PRAGMA_ATTRIBUTE_SUPPORT_LIST)\n";
+  for (const auto *Attr : Records.getAllDerivedDefinitions("Attr")) {
+if (!Attr->getValueAsBit("SupportsPragmaAttribute"))
+  continue;
+std::vector Spellings = GetFlattenedSpellings(*Attr);
+for (const auto  : Spellings)
+  OS << ".Case(\"" << I.name() << "\", 1)\n";
+  }
+  OS << "#endif // CLANG_ATTR_PRAGMA_ATTRIBUTE_SUPPORT_LIST\n\n";
+}
+
 template 
 static void forEachUniqueSpelling(const Record , Fn &) {
   std::vector Spellings = GetFlattenedSpellings(Attr);
@@ -2692,9 +2706,13 @@
   return B + "Decl";
 }
 
+static std::string functionNameForCustomAppertainsTo(const Record ) {
+  return "is" + Subject.getName().str();
+}
+
 static std::string GenerateCustomAppertainsTo(const Record ,
   raw_ostream ) {
-  std::string FnName = "is" + Subject.getName().str();
+  std::string FnName = functionNameForCustomAppertainsTo(Subject);
 
   // If this code has already been generated, simply return the previous
   // instance of it.
@@ -2779,6 +2797,51 @@
   return FnName;
 }
 
+static void GenerateDefaultAppliesTo(raw_ostream ) {
+  OS << "static bool defaultAttributeAppliesTo(Sema &, const Decl *) {\n";
+  OS << "  return true;\n";
+  OS << "}\n\n";
+}
+
+static std::string GenerateAppliesTo(const Record , raw_ostream ) {
+  // If the attribute doesn't support '#pragma clang attribute' or if it doesn't
+  // contain a subjects definition, then use the default appliedTo logic.
+  if (!Attr.getValueAsBit("SupportsPragmaAttribute") ||
+  Attr.isValueUnset("Subjects"))
+return "defaultAttributeAppliesTo";
+
+  const Record *SubjectObj = Attr.getValueAsDef("Subjects");
+  std::vector Subjects = SubjectObj->getValueAsListOfDefs("Subjects");
+
+  // If the list of subjects is empty, it is assumed that the attribute applies
+  // to everything.
+  if (Subjects.empty())
+return "defaultAttributeAppliesTo";
+
+  // Otherwise, generate an appliesTo check specific to this attribute which
+  // checks all of the given subjects against the Decl passed in. Return the
+  // name of that check to the caller.
+  std::string FnName = "check" + Attr.getName().str() + "AttributeAppliesTo";
+  std::stringstream SS;
+  SS << "static bool " << FnName << "(Sema , const Decl *D) {\n";
+  SS << "  return ";
+  for (auto I = Subjects.begin(), E = Subjects.end(); I != E; ++I) {
+// If the subject has custom code associated with it, use the function
+// that was generated for GenerateAppertainsTo to check if the declaration
+// is valid.
+if ((*I)->isSubClassOf("SubsetSubject"))
+  SS << functionNameForCustomAppertainsTo(**I) << "(D)";
+else
+  SS << "isa<" << GetSubjectWithSuffix(*I) << ">(D)";
+
+if (I + 1 != E)
+  SS << " || ";
+  }
+  SS << ";\n}\n\n";
+  OS << SS.str();
+  return FnName;
+}
+
 static void GenerateDefaultLangOptRequirements(raw_ostream ) {
   OS <<