Committed as r171757 with suggested fixes. Thanks for the review! Michael
On Fri, Jan 4, 2013 at 2:39 PM, Richard Smith <[email protected]> wrote: > On Fri, Jan 4, 2013 at 11:01 AM, Michael Han <[email protected]> > 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 <[email protected]> > > 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 <[email protected] > > > >> wrote: > >> > Reviving an old patch. Is this patch OK to commit? > >> > > >> > Cheers > >> > Michael > >> > > >> > > >> > On Thu, Dec 6, 2012 at 9:39 AM, Michael Han < > [email protected]> > >> > 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 > >> >> <[email protected]> > >> >> wrote: > >> >>> > >> >>> I see. Thanks for the explanation! > >> >>> > >> >>> Michael > >> >>> > >> >>> On Mon, Dec 3, 2012 at 9:38 AM, Jordan Rose <[email protected]> > >> >>> 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 <[email protected]> > >> >>>> 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 < > [email protected]> > >> >>>> wrote: > >> >>>>> > >> >>>>> On Sat, Dec 1, 2012 at 6:30 AM, Michael Han > >> >>>>> <[email protected]> > >> >>>>> 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 <[email protected] > >*/ > >> >>>> > >> >>>> > >> >>>> <fixit.patch>_______________________________________________ > >> >>>> cfe-commits mailing list > >> >>>> [email protected] > >> >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >> >>>> > >> >>>> > >> >>> > >> >> > >> > > > > > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
