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

[Michael Han]

Committed as r171757 with suggested fixes. Thanks for the review!

Michael

On Fri, Jan 4, 2013 at 2:39 PM, Richard Smith <richard at metafoo.co.uk> wrote:

> 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
> >> >>>>
> >> >>>>
> >> >>>
> >> >>
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130107/dc7a0ad9/attachment.html>