Committed as r171757 with suggested fixes. Thanks for the review!<br><br>Michael<br><br><div class="gmail_quote">On Fri, Jan 4, 2013 at 2:39 PM, Richard Smith <span dir="ltr"><<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Fri, Jan 4, 2013 at 11:01 AM, Michael Han <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>> wrote:<br>
> Hi Richard,<br>
><br>
</div><div class="im">> Updated patch attached, please take a look.<br>
<br>
</div>Thanks! Just a couple of small things, then this is ready to go:<br>
<br>
- void ParseCXXMemberSpecification(SourceLocation StartLoc, unsigned TagType,<br>
+ void ParseCXXMemberSpecification(SourceLocation StartLoc,<br>
+ SourceLocation AttrLoc,<br>
+ ParsedAttributesWithRange &Attrs,<br>
+ unsigned TagType,<br>
<br>
How about renaming AttrLoc to AttrFixitLoc? At the moment, it's not<br>
clear that this is 'where the attributes should go', rather than<br>
'where the attributes were found'.<br>
<br>
- if (getLangOpts().CPlusPlus)<br>
- ParseCXXMemberSpecification(StartLoc, TagType, TagOrTempResult.get());<br>
+ if (getLangOpts().CPlusPlus) {<br>
+ ParsedAttributesWithRange Attributes(AttrFactory);<br>
+ ParseCXXMemberSpecification(StartLoc, AttrLoc, Attributes, TagType,<br>
+ TagOrTempResult.get());<br>
+<br>
+ // Recover by adding attributes parsed from ParseCXXMemberSpecification<br>
+ // to the attribute list of the class so they can be applied on the class<br>
+ // later.<br>
+ attrs.takeAllFrom(Attributes);<br>
<br>
I think it would be cleaner to pass 'attrs' directly to<br>
ParseCXXMemberSpecification here (and move the additional<br>
ParsedAttributesWithRange variable and takeAllFrom into there). Among<br>
other things, that'll make it more clear what we're recovering from<br>
here.<br>
<div class="HOEnZb"><div class="h5"><br>
> Michael<br>
><br>
> On Tue, Dec 18, 2012 at 12:49 AM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>><br>
> wrote:<br>
>><br>
>> Sorry for the delay!<br>
>><br>
>> +void Parser::CutAndPasteCode(SourceRange Range,<br>
>> + SourceLocation InsertLocation,<br>
>> + unsigned int DiagID) {<br>
>> + SourceManager &SM = PP.getSourceManager();<br>
>> + const char *Start = SM.getCharacterData(Range.getBegin());<br>
>> + const char *End = SM.getCharacterData(Range.getEnd());<br>
>> + SmallString<64> Code(Start, End);<br>
>><br>
>> This isn't going to work if the ends of the range point into different<br>
>> FileIDs. In any case, it's unnecessary; use<br>
>> FixItHint::CreateInsertionFromRange instead.<br>
>><br>
>> + if (Attributes.Range.isValid()) {<br>
>> + CutAndPasteCode(Attributes.Range, AttrLoc,<br>
>> + diag::err_attributes_not_allowed);<br>
>> + Attributes.clear();<br>
>> + }<br>
>><br>
>> Clearing the attributes here doesn't look right: after issuing a<br>
>> fix-it for an error, you must recover as if the fix-it were applied<br>
>> (so the attributes must actually be applied to the class).<br>
>><br>
>> + // any attributes after class-name, we try a fixit to move<br>
>> + // them to right place.<br>
>><br>
>> ... to *the* right place.<br>
>><br>
>> + // Parse any C++11 attributes after 'final'keyword<br>
>><br>
>> Missing space after 'final'. Missing full stop.<br>
>><br>
>> On Mon, Dec 17, 2012 at 9:56 PM, Michael Han <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> wrote:<br>
>> > Reviving an old patch. Is this patch OK to commit?<br>
>> ><br>
>> > Cheers<br>
>> > Michael<br>
>> ><br>
>> ><br>
>> > On Thu, Dec 6, 2012 at 9:39 AM, Michael Han <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Hi Richard,<br>
>> >><br>
>> >> Do you also have a chance to look at this patch? Please let me know if<br>
>> >> it's OK to commit it. Thanks!<br>
>> >><br>
>> >> Michael<br>
>> >><br>
>> >> On Mon, Dec 3, 2012 at 11:00 AM, Michael Han<br>
>> >> <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> >> wrote:<br>
>> >>><br>
>> >>> I see. Thanks for the explanation!<br>
>> >>><br>
>> >>> Michael<br>
>> >>><br>
>> >>> On Mon, Dec 3, 2012 at 9:38 AM, Jordan Rose <<a href="mailto:jordan_rose@apple.com">jordan_rose@apple.com</a>><br>
>> >>> wrote:<br>
>> >>>><br>
>> >>>> Some of them are primarily testing the caret fixits, but you're right<br>
>> >>>> that the parseable fixits don't actually prevent you from testing the<br>
>> >>>> caret<br>
>> >>>> fixits. On the other hand, until recently FileCheck didn't have a way<br>
>> >>>> to<br>
>> >>>> specify relative line numbers, meaning that adding a new line at the<br>
>> >>>> top of<br>
>> >>>> the file would change all of the parseable fixits (unless matched<br>
>> >>>> with<br>
>> >>>> wildcards).<br>
>> >>>><br>
>> >>>> Going forward we should probably endeavour to test the parseable<br>
>> >>>> fixits<br>
>> >>>> for every test where we're not just testing caret fixit emission.<br>
>> >>>><br>
>> >>>> Jordan<br>
>> >>>><br>
>> >>>><br>
>> >>>> On Dec 1, 2012, at 21:05 , Michael Han <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> >>>> wrote:<br>
>> >>>><br>
>> >>>> Hi Dmitri,<br>
>> >>>><br>
>> >>>> Thanks! Attach updated patch.<br>
>> >>>><br>
>> >>>> I noticed that not all test cases under clang/test/FixIt use<br>
>> >>>> "-fdiagnostics-parseable-fixits". Is there a reason why not all FixIt<br>
>> >>>> test<br>
>> >>>> cases use this option?<br>
>> >>>><br>
>> >>>> Cheers<br>
>> >>>> Michael<br>
>> >>>><br>
>> >>>> On Sat, Dec 1, 2012 at 5:21 AM, Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>><br>
>> >>>> wrote:<br>
>> >>>>><br>
>> >>>>> On Sat, Dec 1, 2012 at 6:30 AM, Michael Han<br>
>> >>>>> <<a href="mailto:fragmentshaders@gmail.com">fragmentshaders@gmail.com</a>><br>
>> >>>>> wrote:<br>
>> >>>>> > This patch adds two FixIts to parser when parsing C++11 attributes<br>
>> >>>>> > appertain<br>
>> >>>>> > to class specifier. It is a following up patch of a previous patch<br>
>> >>>>> > [1].<br>
>> >>>>> ><br>
>> >>>>> > Please review, thanks!<br>
>> >>>>><br>
>> >>>>> Hello Michael,<br>
>> >>>>><br>
>> >>>>> FixIts can be tested with -fdiagnostics-parseable-fixits (search the<br>
>> >>>>> testsuite for this option to find examples).<br>
>> >>>>><br>
>> >>>>> Dmitri<br>
>> >>>>><br>
>> >>>>> --<br>
>> >>>>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if<br>
>> >>>>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <<a href="mailto:gribozavr@gmail.com">gribozavr@gmail.com</a>>*/<br>
>> >>>><br>
>> >>>><br>
>> >>>> <fixit.patch>_______________________________________________<br>
>> >>>> cfe-commits mailing list<br>
>> >>>> <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
>> >>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
>> >>>><br>
>> >>>><br>
>> >>><br>
>> >><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br>