|
|
Patch Set 1 #
Total comments: 16
Total messages: 3
|
supertri
I think the patch is looking really good! We can see if Jeffrey or Chandler ...
|
14 years, 4 months ago (2011年08月24日 01:28:00 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
I think the patch is looking really good! We can see if Jeffrey or Chandler have any cosmetic edits tomorrow, and then send it out for review upstream. http://codereview.appspot.com/4932050/diff/1/include/clang/Parse/Parser.h File include/clang/Parse/Parser.h (right): http://codereview.appspot.com/4932050/diff/1/include/clang/Parse/Parser.h#new... include/clang/Parse/Parser.h:680: /// FIXME: Perhaps we should change the name of LateParsedDeclaration to We should be sure to ask Doug about this FIXME when he is reviewing the patch. http://codereview.appspot.com/4932050/diff/1/include/clang/Parse/Parser.h#new... include/clang/Parse/Parser.h:690: SourceLocation Loc) : Self(P), Indentation is weird here. http://codereview.appspot.com/4932050/diff/1/include/clang/Parse/Parser.h#new... include/clang/Parse/Parser.h:700: typedef llvm::SmallVector<LateParsedAttribute*, 4> LateParsedAttrList; I think size 1 would be a better base case. http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp File lib/Parse/ParseDecl.cpp (right): http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode96 lib/Parse/ParseDecl.cpp:96: LateParsedAttrList *LAS) { LAS? http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode679 lib/Parse/ParseDecl.cpp:679: // FIXME: Test cases to make sure this does the right thing in this context Can we add a test case involving templates? http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode701 lib/Parse/ParseDecl.cpp:701: /// FIXME: Need to add to the decl correctly I think this FIXME has been fixed. :) http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode721 lib/Parse/ParseDecl.cpp:721: // taken directly from existing attribute argument processing code I now know (and try to remember) that comments should be complete sentences, even ending with a period. http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode742 lib/Parse/ParseDecl.cpp:742: endLoc = Tok.getLocation(); //FIXME: is this correct endLoc? Any thoughts on the FIXME? I think I wrote this. Could leave it in as question for Doug or remove if you think it does the right thing. http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode750 lib/Parse/ParseDecl.cpp:750: if (LA.D) Does it sometimes happen that !LA.D? Wouldn't that be very bad? http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode809 lib/Parse/ParseDecl.cpp:809: // FIXME: Figure out how to pass along Attrs appropriately I think this FIXME has been dealt with... http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode840 lib/Parse/ParseDecl.cpp:840: SkipUntil(tok::r_paren); I think this should be MatchRHSPunctuation everywhere. Maybe we should commit a patch refactoring this loop into a method before committing the delayed parsing patch. :) http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDeclCXX.cpp File lib/Parse/ParseDeclCXX.cpp (right): http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDeclCXX.cpp#newco... lib/Parse/ParseDeclCXX.cpp:2168: // declarations and the lexed inline method definitions. Add "along with any delayed attributes" to comment?
http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp File lib/Parse/ParseDecl.cpp (right): http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode96 lib/Parse/ParseDecl.cpp:96: LateParsedAttrList *LAS) { On 2011年08月24日 01:28:00, supertri wrote: > LAS? One late attribute = LA. Two late attributes = LAS. :-) http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode679 lib/Parse/ParseDecl.cpp:679: // FIXME: Test cases to make sure this does the right thing in this context On 2011年08月24日 01:28:00, supertri wrote: > Can we add a test case involving templates? We probably should. http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode750 lib/Parse/ParseDecl.cpp:750: if (LA.D) On 2011年08月24日 01:28:00, supertri wrote: > Does it sometimes happen that !LA.D? Wouldn't that be very bad? It indicates that the attribute was not attached to the Decl correctly. If that happens, we have two choices: complain with an assert, indicating an internal compiler error, or ignore the attribute and fail gracefully. Which do you prefer?
http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp File lib/Parse/ParseDecl.cpp (right): http://codereview.appspot.com/4932050/diff/1/lib/Parse/ParseDecl.cpp#newcode96 lib/Parse/ParseDecl.cpp:96: LateParsedAttrList *LAS) { On 2011年08月24日 22:06:24, delesley wrote: > On 2011年08月24日 01:28:00, supertri wrote: > > LAS? > > One late attribute = LA. Two late attributes = LAS. :-) LateAttrs would be much more readable... At the ver yleast it should be LAs, but thats far to confusing for my taste.