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

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

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

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

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

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 >

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

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:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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 ___

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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:

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

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

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

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

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

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

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

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

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

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

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

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

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

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