Patch for delayed attribute parsing.

[supertri, 2011-08-24 01:28:00]

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?

[delesley, 2011-08-24 22:06:24]

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?

[chandlerc, 2011-08-24 22:10:08]

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.