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

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

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

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