<div>On Wed Jan 29 2014 at 5:36:44 AM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Wed, Jan 29, 2014 at 2:39 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>> wrote:<br>
><br>
> On 29/01/2014 06:30, Richard Smith wrote:<br>
><br>
> On Tue, Jan 28, 2014 at 4:58 PM, Joseph Mansfield <<a href="mailto:sftrabbit@gmail.com" target="_blank">sftrabbit@gmail.com</a>><br>
> wrote:<br>
>><br>
>> The attached patch adds support for deprecation messages in C++1y. This is<br>
>> my first time hacking with clang so a code review would definitely be<br>
>> needed.<br>
>><br>
>> As an example of what is now supported:<br>
>><br>
>> [[deprecated("use bar instead")]] void foo();<br>
>><br>
>> When this function is used, the following warning is emitted:<br>
>><br>
>> warning: 'foo' is deprecated: use bar instead [-Wdeprecated-declarations]<br>
>>   foo();<br>
>>   ^<br>
>><br>
>> Some tests included.<br>
>><br>
>> Known potential issues:<br>
>> - Attribute parsing currently doesn't seem to distinguish between C++11<br>
>> and C++1y attributes.<br>
>> - Maybe errors like [[deprecated("foo", "bar")]] would better report "too<br>
>> many arguments" than "expected ')'". Not sure about the best way to go about<br>
>> this.<br>
><br>
><br>
> Thanks for the patch! The code generally looks good from a style<br>
> perspective, but please start variable names with a capital letter in new<br>
> code (the current codebase is woefully inconsistent, but we're trying to get<br>
> new code to follow the style guide). (The style guide also says to start<br>
> functions with a lowercase letter, but we're not doing that for Parse*<br>
> because it's such a well-established pattern that the inconsistency would be<br>
> worse than following the style guide. We'll get around to doing a mass<br>
> cleanup of this stuff one day...)<br>
><br>
> We've been trying to move away from having a collection of different<br>
> attribute argument parsing functions, and towards generating the parsing<br>
> logic from the Attr.td file -- I'll let Aaron weigh in on whether we want<br>
> something like ParseAttributeWithMessageArg or whether we should generalize<br>
> and reuse ParseGNUAttributeArgs.<br>
><br>
><br>
> Indeed, this should work already if you remove the check (ScopeName &&<br>
> ScopeName->getName() == "gnu"):<br>
<br>
Alp hit the nail on the head -- the parsing is already handled for the<br>
GNU case, we can simply reuse it.<br></blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'm not certain we need the IsBuiltInOrStandardCXX11Attrib<u></u>ute function<br>
any longer -- I think this should be declarative-based and handled as<br>
part of sema, not parsing (in terms of complaining about repeated<br>
attributes) since it really depends on the attribute, not the<br>
spelling. Not that it would block this patch.<br>
<br>
The new spelling does not appear in Attr.td, which is the only thing I<br>
really see missing.<br></blockquote><div><br></div><div>It's already there and working -- this patch is only adding support for doing something with the attribute's argument.</div>