clang: C++1y deprecated attribute message

[Aaron Ballman]

On Wed, Jan 29, 2014 at 2:34 PM, Richard Smith <metafoo at gmail.com> wrote:
> On Wed Jan 29 2014 at 11:22:29 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Wed, Jan 29, 2014 at 2:04 PM, Richard Smith <metafoo at gmail.com> wrote:
>> > On Wed Jan 29 2014 at 10:35:46 AM, Aaron Ballman
>> > <aaron at aaronballman.com>
>> > wrote:
>> >>
>> >> On Wed, Jan 29, 2014 at 1:07 PM, Richard Smith <metafoo at gmail.com>
>> >> wrote:
>> >> > On Wed Jan 29 2014 at 5:36:44 AM, Aaron Ballman
>> >> > <aaron at aaronballman.com>
>> >> > wrote:
>> >> >>
>> >> >> On Wed, Jan 29, 2014 at 2:39 AM, Alp Toker <alp at nuanti.com> wrote:
>> >> >> >
>> >> >> > On 29/01/2014 06:30, Richard Smith wrote:
>> >> >> >
>> >> >> > On Tue, Jan 28, 2014 at 4:58 PM, Joseph Mansfield
>> >> >> > <sftrabbit at gmail.com>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> The attached patch adds support for deprecation messages in
>> >> >> >> C++1y.
>> >> >> >> This
>> >> >> >> is
>> >> >> >> my first time hacking with clang so a code review would
>> >> >> >> definitely
>> >> >> >> be
>> >> >> >> needed.
>> >> >> >>
>> >> >> >> As an example of what is now supported:
>> >> >> >>
>> >> >> >> [[deprecated("use bar instead")]] void foo();
>> >> >> >>
>> >> >> >> When this function is used, the following warning is emitted:
>> >> >> >>
>> >> >> >> warning: 'foo' is deprecated: use bar instead
>> >> >> >> [-Wdeprecated-declarations]
>> >> >> >>   foo();
>> >> >> >>   ^
>> >> >> >>
>> >> >> >> Some tests included.
>> >> >> >>
>> >> >> >> Known potential issues:
>> >> >> >> - Attribute parsing currently doesn't seem to distinguish between
>> >> >> >> C++11
>> >> >> >> and C++1y attributes.
>> >> >> >> - Maybe errors like [[deprecated("foo", "bar")]] would better
>> >> >> >> report
>> >> >> >> "too
>> >> >> >> many arguments" than "expected ')'". Not sure about the best way
>> >> >> >> to
>> >> >> >> go
>> >> >> >> about
>> >> >> >> this.
>> >> >> >
>> >> >> >
>> >> >> > Thanks for the patch! The code generally looks good from a style
>> >> >> > perspective, but please start variable names with a capital letter
>> >> >> > in
>> >> >> > new
>> >> >> > code (the current codebase is woefully inconsistent, but we're
>> >> >> > trying
>> >> >> > to
>> >> >> > get
>> >> >> > new code to follow the style guide). (The style guide also says to
>> >> >> > start
>> >> >> > functions with a lowercase letter, but we're not doing that for
>> >> >> > Parse*
>> >> >> > because it's such a well-established pattern that the
>> >> >> > inconsistency
>> >> >> > would be
>> >> >> > worse than following the style guide. We'll get around to doing a
>> >> >> > mass
>> >> >> > cleanup of this stuff one day...)
>> >> >> >
>> >> >> > We've been trying to move away from having a collection of
>> >> >> > different
>> >> >> > attribute argument parsing functions, and towards generating the
>> >> >> > parsing
>> >> >> > logic from the Attr.td file -- I'll let Aaron weigh in on whether
>> >> >> > we
>> >> >> > want
>> >> >> > something like ParseAttributeWithMessageArg or whether we should
>> >> >> > generalize
>> >> >> > and reuse ParseGNUAttributeArgs.
>> >> >> >
>> >> >> >
>> >> >> > Indeed, this should work already if you remove the check
>> >> >> > (ScopeName
>> >> >> > &&
>> >> >> > ScopeName->getName() == "gnu"):
>> >> >>
>> >> >> Alp hit the nail on the head -- the parsing is already handled for
>> >> >> the
>> >> >> GNU case, we can simply reuse it.
>> >> >
>> >> >
>> >> > Alp's suggestion needs some refinement -- the standard attributes
>> >> > (other
>> >> > than 'deprecated') can't take arguments at all (not even empty
>> >> > parentheses),
>> >> > so we'll presumably need some way to encode that in the .td file if
>> >> > we
>> >> > want
>> >> > to move away from the hardcoded list.
>> >>
>> >> Ah, so this is a parsing issue as well then.
>> >>
>> >> Perhaps we could use another meta-spelling: StdCXX that expands to:
>> >> CXX11<"", name> and CXX11<"std", name>?
>> >>
>> >> >>
>> >> >> I'm not certain we need the IsBuiltInOrStandardCXX11Attribute
>> >> >> function
>> >> >> any longer -- I think this should be declarative-based and handled
>> >> >> as
>> >> >> part of sema, not parsing (in terms of complaining about repeated
>> >> >> attributes) since it really depends on the attribute, not the
>> >> >> spelling. Not that it would block this patch.
>> >> >>
>> >> >> The new spelling does not appear in Attr.td, which is the only thing
>> >> >> I
>> >> >> really see missing.
>> >> >
>> >> >
>> >> > It's already there and working -- this patch is only adding support
>> >> > for
>> >> > doing something with the attribute's argument.
>> >>
>> >> It has no std scope spelling, which I presume is problematic?
>> >
>> >
>> > I don't see any point in the 'std' attribute namespace, and looking
>> > through
>> > svn history, I can't see any justification being provided when it was
>> > added.
>> > Just remove those spellings entirely?
>>
>> I see nothing in the standard which suggests these should be in the
>> std:: namespace, so I'm in favor of removing those spellings.
>>
>> Then again, I also don't see anything in the standard claiming that
>> unscoped attributes are reserved which makes me fearful for naming
>> collisions with different semantics between implementations. :-(
>
>
> They're intended to be available for future standardization, with (IIRC) the
> intent that attributes from C++1y are a conforming extension in C++11, so
> they can't /really/ be reserved and still support that use case. (They could
> say something like "reserved for future standardization", but that doesn't
> quite capture the intent as I understand it -- they really want "other
> unscoped attributes are conditionally-supported, with semantics as specified
> in some unknown future C++ standard".)
>
> We have (non-normative) encouragement to put non-standard attributes in our
> own attribute namespace (see the note in 7.6.1/3).

I've put out a patch that makes this a bit more declarative, and
leaves the door open to easily adding a std:: namespace automatically,
were that to become desirable in the future. It has little to do with
this patch though, so I've posted it under a new thread.

~Aaron