[cfe-commits] [PATCH] Add Fixit for C++11 attributes that appertain to class specifiers.

[Richard Smith]

On Fri, Jan 4, 2013 at 11:01 AM, Michael Han <fragmentshaders at gmail.com> wrote:
> Hi Richard,
>
> Updated patch attached, please take a look.

Thanks! Just a couple of small things, then this is ready to go:

-  void ParseCXXMemberSpecification(SourceLocation StartLoc, unsigned TagType,
+  void ParseCXXMemberSpecification(SourceLocation StartLoc,
+                                   SourceLocation AttrLoc,
+                                   ParsedAttributesWithRange &Attrs,
+                                   unsigned TagType,

How about renaming AttrLoc to AttrFixitLoc? At the moment, it's not
clear that this is 'where the attributes should go', rather than
'where the attributes were found'.

-    if (getLangOpts().CPlusPlus)
-      ParseCXXMemberSpecification(StartLoc, TagType, TagOrTempResult.get());
+    if (getLangOpts().CPlusPlus) {
+      ParsedAttributesWithRange Attributes(AttrFactory);
+      ParseCXXMemberSpecification(StartLoc, AttrLoc, Attributes, TagType,
+                                  TagOrTempResult.get());
+
+      // Recover by adding attributes parsed from ParseCXXMemberSpecification
+      // to the attribute list of the class so they can be applied on the class
+      // later.
+      attrs.takeAllFrom(Attributes);

I think it would be cleaner to pass 'attrs' directly to
ParseCXXMemberSpecification here (and move the additional
ParsedAttributesWithRange variable and takeAllFrom into there). Among
other things, that'll make it more clear what we're recovering from
here.

> Michael
>
> On Tue, Dec 18, 2012 at 12:49 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
>>
>> Sorry for the delay!
>>
>> +void Parser::CutAndPasteCode(SourceRange Range,
>> +                             SourceLocation InsertLocation,
>> +                             unsigned int DiagID) {
>> +  SourceManager &SM = PP.getSourceManager();
>> +  const char *Start = SM.getCharacterData(Range.getBegin());
>> +  const char *End = SM.getCharacterData(Range.getEnd());
>> +  SmallString<64> Code(Start, End);
>>
>> This isn't going to work if the ends of the range point into different
>> FileIDs. In any case, it's unnecessary; use
>> FixItHint::CreateInsertionFromRange instead.
>>
>> +    if (Attributes.Range.isValid()) {
>> +      CutAndPasteCode(Attributes.Range, AttrLoc,
>> +                      diag::err_attributes_not_allowed);
>> +      Attributes.clear();
>> +    }
>>
>> Clearing the attributes here doesn't look right: after issuing a
>> fix-it for an error, you must recover as if the fix-it were applied
>> (so the attributes must actually be applied to the class).
>>
>> +    // any attributes after class-name, we try a fixit to move
>> +    // them to right place.
>>
>> ... to *the* right place.
>>
>> +    // Parse any C++11 attributes after 'final'keyword
>>
>> Missing space after 'final'. Missing full stop.
>>
>> On Mon, Dec 17, 2012 at 9:56 PM, Michael Han <fragmentshaders at gmail.com>
>> wrote:
>> > Reviving an old patch. Is this patch OK to commit?
>> >
>> > Cheers
>> > Michael
>> >
>> >
>> > On Thu, Dec 6, 2012 at 9:39 AM, Michael Han <fragmentshaders at gmail.com>
>> > wrote:
>> >>
>> >> Hi Richard,
>> >>
>> >> Do you also have a chance to look at this patch? Please let me know if
>> >> it's OK to commit it. Thanks!
>> >>
>> >> Michael
>> >>
>> >> On Mon, Dec 3, 2012 at 11:00 AM, Michael Han
>> >> <fragmentshaders at gmail.com>
>> >> wrote:
>> >>>
>> >>> I see. Thanks for the explanation!
>> >>>
>> >>> Michael
>> >>>
>> >>> On Mon, Dec 3, 2012 at 9:38 AM, Jordan Rose <jordan_rose at apple.com>
>> >>> wrote:
>> >>>>
>> >>>> Some of them are primarily testing the caret fixits, but you're right
>> >>>> that the parseable fixits don't actually prevent you from testing the
>> >>>> caret
>> >>>> fixits. On the other hand, until recently FileCheck didn't have a way
>> >>>> to
>> >>>> specify relative line numbers, meaning that adding a new line at the
>> >>>> top of
>> >>>> the file would change all of the parseable fixits (unless matched
>> >>>> with
>> >>>> wildcards).
>> >>>>
>> >>>> Going forward we should probably endeavour to test the parseable
>> >>>> fixits
>> >>>> for every test where we're not just testing caret fixit emission.
>> >>>>
>> >>>> Jordan
>> >>>>
>> >>>>
>> >>>> On Dec 1, 2012, at 21:05 , Michael Han <fragmentshaders at gmail.com>
>> >>>> wrote:
>> >>>>
>> >>>> Hi Dmitri,
>> >>>>
>> >>>> Thanks! Attach updated patch.
>> >>>>
>> >>>> I noticed that not all test cases under clang/test/FixIt use
>> >>>> "-fdiagnostics-parseable-fixits". Is there a reason why not all FixIt
>> >>>> test
>> >>>> cases use this option?
>> >>>>
>> >>>> Cheers
>> >>>> Michael
>> >>>>
>> >>>> On Sat, Dec 1, 2012 at 5:21 AM, Dmitri Gribenko <gribozavr at gmail.com>
>> >>>> wrote:
>> >>>>>
>> >>>>> On Sat, Dec 1, 2012 at 6:30 AM, Michael Han
>> >>>>> <fragmentshaders at gmail.com>
>> >>>>> wrote:
>> >>>>> > This patch adds two FixIts to parser when parsing C++11 attributes
>> >>>>> > appertain
>> >>>>> > to class specifier. It is a following up patch of a previous patch
>> >>>>> > [1].
>> >>>>> >
>> >>>>> > Please review, thanks!
>> >>>>>
>> >>>>> Hello Michael,
>> >>>>>
>> >>>>> FixIts can be tested with -fdiagnostics-parseable-fixits (search the
>> >>>>> testsuite for this option to find examples).
>> >>>>>
>> >>>>> Dmitri
>> >>>>>
>> >>>>> --
>> >>>>> main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
>> >>>>> (j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/
>> >>>>
>> >>>>
>> >>>> <fixit.patch>_______________________________________________
>> >>>> cfe-commits mailing list
>> >>>> cfe-commits at cs.uiuc.edu
>> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >>>>
>> >>>>
>> >>>
>> >>
>> >
>
>