Added support for late parsing of attribute arguments to GNU attributes.

[supertri, 2011-09-01 18:32:38]

http://codereview.appspot.com/4975049/diff/1/lib/Parse/ParseDecl.cpp
File lib/Parse/ParseDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/1/lib/Parse/ParseDecl.cpp#newcode106
lib/Parse/ParseDecl.cpp:106: LateParsedAttrList *LAS) {
There was a previous discussion on this variable name. Looks like you have to
merge in changes from the previous patch sent to Doug.

http://codereview.appspot.com/4975049/diff/1/lib/Parse/ParseDecl.cpp#newcode177
lib/Parse/ParseDecl.cpp:177: {
{ on previous line
attrs => Attrs
endLoc => EndLoc

http://codereview.appspot.com/4975049/diff/1/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):

http://codereview.appspot.com/4975049/diff/1/lib/Sema/AnalysisBasedWarnings.c...
lib/Sema/AnalysisBasedWarnings.cpp:737: } else if (CXXThisExpr *TE =
dyn_cast<CXXThisExpr>(Exp)) {
This adds a warning for an unused variable TE.

[delesley, 2011-09-01 20:18:23]

http://codereview.appspot.com/4975049/diff/1/lib/Parse/ParseDecl.cpp
File lib/Parse/ParseDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/1/lib/Parse/ParseDecl.cpp#newcode106
lib/Parse/ParseDecl.cpp:106: LateParsedAttrList *LAS) {
Actually, it looks that this wasn't fixed even in the early patch.  It's fixed
now.  

On 2011/09/01 18:32:38, supertri wrote:
> There was a previous discussion on this variable name. Looks like you have to
> merge in changes from the previous patch sent to Doug.

http://codereview.appspot.com/4975049/diff/1/lib/Parse/ParseDecl.cpp#newcode177
lib/Parse/ParseDecl.cpp:177: {
On 2011/09/01 18:32:38, supertri wrote:
> { on previous line
> attrs => Attrs
> endLoc => EndLoc

Done.

http://codereview.appspot.com/4975049/diff/1/lib/Sema/AnalysisBasedWarnings.cpp
File lib/Sema/AnalysisBasedWarnings.cpp (right):

http://codereview.appspot.com/4975049/diff/1/lib/Sema/AnalysisBasedWarnings.c...
lib/Sema/AnalysisBasedWarnings.cpp:737: } else if (CXXThisExpr *TE =
dyn_cast<CXXThisExpr>(Exp)) {
On 2011/09/01 18:32:38, supertri wrote:
> This adds a warning for an unused variable TE. 

Done.

[chandlerc, 2011-09-01 22:45:45]

http://codereview.appspot.com/4975049/diff/2002/include/clang/Basic/Attr.td
File include/clang/Basic/Attr.td (right):

http://codereview.appspot.com/4975049/diff/2002/include/clang/Basic/Attr.td#n...
include/clang/Basic/Attr.td:93: bit LateParsed=0;
spaces around the '='

http://codereview.appspot.com/4975049/diff/2002/include/clang/Basic/Attr.td#n...
include/clang/Basic/Attr.td:570: let LateParsed=1;
spaces around '=' here and below as well.

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/CMakeList...
File include/clang/Parse/CMakeLists.txt (right):

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/CMakeList...
include/clang/Parse/CMakeLists.txt:5: DEPENDS AttrLateParsed.inc)
I don't think you need this line... at least other clang_tablegen rules don't
have this line.

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/Parser.h
File include/clang/Parse/Parser.h (right):

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/Parser.h#...
include/clang/Parse/Parser.h:690: SourceLocation Loc)
Indent on this line seems off

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/Parser.h#...
include/clang/Parse/Parser.h:698: typedef
llvm::SmallVector<LateParsedAttribute*, 2> LateParsedAttrList;
This needs a doxyment

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp
File lib/Parse/ParseDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:59: /// HasAttribute -  Return true if we recognize and
implement the attribute
This comment is wrong

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:61: static bool isAttributeLateParsed(const
IdentifierInfo *II) {
const reference?

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:136: // Delay parsing of this attribute
Not sure why this comment line is here... seems redundant, especially after
'isAttributeLateParsed' above.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:152: else {
else goes on th eline with the }

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:178: {
{ goes on the line with the )

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:188: else if
(IsThreadSafetyAttribute(AttrName->getName())) {
don't use 'else' when the if returns.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:296: 
Why all the extra blank lines?

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:712: /// Late Parsed Attributes:
This isn't a valid doxygen comment, so you shouldn't use the syntax.

Have you read up on the doxygen comment syntax? I feel like you're just using
'///' without formatting the comments in a way that doxygen can easily extract
them.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:728: 
extraneous blank lines

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:743: for (size_t i = 0; i <
Class.LateParsedDeclarations.size(); ++i) {
The two idiomatic ways to do this is to use 'unsigned' or to use iterators. I
prefer iterators, but there is no real difference here.

However, you need to pull the size computation into a variable rather than doing
it on every iteration.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:754: SourceLocation origLoc = Tok.getLocation();
OrigLoc

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:768: assert(LA.D);
add a message for why this should never happen?

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:826: // Consume the left paren
This comment is rather redundant with the ones 2 lines down...

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDeclCXX.cpp
File lib/Parse/ParseDeclCXX.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDeclCXX.cpp#ne...
lib/Parse/ParseDeclCXX.cpp:1790: }
I would prefer a for loop over the vector calling setDecl, and then clear().

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDeclCXX.cpp#ne...
lib/Parse/ParseDeclCXX.cpp:1894: }
Here as well

http://codereview.appspot.com/4975049/diff/2002/lib/Sema/SemaDecl.cpp
File lib/Sema/SemaDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:6730: void Sema::ActOnFinishDelayedAttribute(Scope *S,
Decl *D, ParsedAttributes &Attrs) {
80 columns

http://codereview.appspot.com/4975049/diff/2002/test/SemaCXX/warn-thread-safe...
File test/SemaCXX/warn-thread-safety-analysis.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/test/SemaCXX/warn-thread-safe...
test/SemaCXX/warn-thread-safety-analysis.cpp:426: }
It would be good to also test warnings on constructs here that require
attributes that only appear later in the class definition.

[delesley, 2011-09-02 18:19:07]

http://codereview.appspot.com/4975049/diff/2002/include/clang/Basic/Attr.td
File include/clang/Basic/Attr.td (right):

http://codereview.appspot.com/4975049/diff/2002/include/clang/Basic/Attr.td#n...
include/clang/Basic/Attr.td:93: bit LateParsed=0;
On 2011/09/01 22:45:45, chandlerc wrote:
> spaces around the '='

Done.

http://codereview.appspot.com/4975049/diff/2002/include/clang/Basic/Attr.td#n...
include/clang/Basic/Attr.td:570: let LateParsed=1;
On 2011/09/01 22:45:45, chandlerc wrote:
> spaces around '=' here and below as well.

Done.

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/CMakeList...
File include/clang/Parse/CMakeLists.txt (right):

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/CMakeList...
include/clang/Parse/CMakeLists.txt:5: DEPENDS AttrLateParsed.inc)
This is based off the clang_tablegen rule for AttrSpellings, which does have the
line.

On 2011/09/01 22:45:45, chandlerc wrote:
> I don't think you need this line... at least other clang_tablegen rules don't
> have this line.

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/Parser.h
File include/clang/Parse/Parser.h (right):

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/Parser.h#...
include/clang/Parse/Parser.h:690: SourceLocation Loc)
On 2011/09/01 22:45:45, chandlerc wrote:
> Indent on this line seems off

Done.

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/Parser.h#...
include/clang/Parse/Parser.h:698: typedef
llvm::SmallVector<LateParsedAttribute*, 2> LateParsedAttrList;
On 2011/09/01 22:45:45, chandlerc wrote:
> This needs a doxyment

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp
File lib/Parse/ParseDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:59: /// HasAttribute -  Return true if we recognize and
implement the attribute
On 2011/09/01 22:45:45, chandlerc wrote:
> This comment is wrong

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:61: static bool isAttributeLateParsed(const
IdentifierInfo *II) {
Adapted from HasAttribute function.  

On 2011/09/01 22:45:45, chandlerc wrote:
> const reference?

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:136: // Delay parsing of this attribute
On 2011/09/01 22:45:45, chandlerc wrote:
> Not sure why this comment line is here... seems redundant, especially after
> 'isAttributeLateParsed' above.

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:152: else {
On 2011/09/01 22:45:45, chandlerc wrote:
> else goes on th eline with the }

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:178: {
On 2011/09/01 22:45:45, chandlerc wrote:
> { goes on the line with the )

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:188: else if
(IsThreadSafetyAttribute(AttrName->getName())) {
On 2011/09/01 22:45:45, chandlerc wrote:
> don't use 'else' when the if returns.

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:296: 
On 2011/09/01 22:45:45, chandlerc wrote:
> Why all the extra blank lines?

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:712: /// Late Parsed Attributes:
On 2011/09/01 22:45:45, chandlerc wrote:
> This isn't a valid doxygen comment, so you shouldn't use the syntax.
> 
> Have you read up on the doxygen comment syntax? I feel like you're just using
> '///' without formatting the comments in a way that doxygen can easily extract
> them.

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:728: 
On 2011/09/01 22:45:45, chandlerc wrote:
> extraneous blank lines

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:743: for (size_t i = 0; i <
Class.LateParsedDeclarations.size(); ++i) {
On 2011/09/01 22:45:45, chandlerc wrote:
> The two idiomatic ways to do this is to use 'unsigned' or to use iterators. I
> prefer iterators, but there is no real difference here.
> 
> However, you need to pull the size computation into a variable rather than
doing
> it on every iteration.

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:754: SourceLocation origLoc = Tok.getLocation();
On 2011/09/01 22:45:45, chandlerc wrote:
> OrigLoc

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:768: assert(LA.D);
On 2011/09/01 22:45:45, chandlerc wrote:
> add a message for why this should never happen?

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:826: // Consume the left paren
On 2011/09/01 22:45:45, chandlerc wrote:
> This comment is rather redundant with the ones 2 lines down...

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDeclCXX.cpp
File lib/Parse/ParseDeclCXX.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDeclCXX.cpp#ne...
lib/Parse/ParseDeclCXX.cpp:1790: }
On 2011/09/01 22:45:45, chandlerc wrote:
> I would prefer a for loop over the vector calling setDecl, and then clear().

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDeclCXX.cpp#ne...
lib/Parse/ParseDeclCXX.cpp:1894: }
On 2011/09/01 22:45:45, chandlerc wrote:
> Here as well

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Sema/SemaDecl.cpp
File lib/Sema/SemaDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/lib/Sema/SemaDecl.cpp#newcode...
lib/Sema/SemaDecl.cpp:6730: void Sema::ActOnFinishDelayedAttribute(Scope *S,
Decl *D, ParsedAttributes &Attrs) {
On 2011/09/01 22:45:45, chandlerc wrote:
> 80 columns

Done.

http://codereview.appspot.com/4975049/diff/2002/test/SemaCXX/warn-thread-safe...
File test/SemaCXX/warn-thread-safety-analysis.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/test/SemaCXX/warn-thread-safe...
test/SemaCXX/warn-thread-safety-analysis.cpp:426: }
On 2011/09/01 22:45:45, chandlerc wrote:
> It would be good to also test warnings on constructs here that require
> attributes that only appear later in the class definition.

Done.

[chandlerc, 2011-09-02 18:45:30]

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/CMakeList...
File include/clang/Parse/CMakeLists.txt (right):

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/CMakeList...
include/clang/Parse/CMakeLists.txt:5: DEPENDS AttrLateParsed.inc)
On 2011/09/02 18:19:08, delesley wrote:
> This is based off the clang_tablegen rule for AttrSpellings, which does have
the
> line.

That is literally the only use clang_tablegen which has this. If you read the
actual documentation for clang_tablegen, it clearly lists this field as for
"extra-dependencies". I'll fix AttrSpellings in a bit.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp
File lib/Parse/ParseDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:61: static bool isAttributeLateParsed(const
IdentifierInfo *II) {
On 2011/09/02 18:19:08, delesley wrote:
> Adapted from HasAttribute function.

Ok, but would a const reference make more sense?

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:768: assert(LA.D);
On 2011/09/02 18:19:08, delesley wrote:
> On 2011/09/01 22:45:45, chandlerc wrote:
> > add a message for why this should never happen?
> 
> Done.

In general, it helps to condense the message into a string that the assert
prints out. I was trying to suggest something like:

assert(LA.D && "No decl attached to late parsed attribute");

http://codereview.appspot.com/4975049/diff/2002/test/SemaCXX/warn-thread-safe...
File test/SemaCXX/warn-thread-safety-analysis.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/test/SemaCXX/warn-thread-safe...
test/SemaCXX/warn-thread-safety-analysis.cpp:426: }
On 2011/09/02 18:19:08, delesley wrote:
> On 2011/09/01 22:45:45, chandlerc wrote:
> > It would be good to also test warnings on constructs here that require
> > attributes that only appear later in the class definition.
> 
> Done.

Er, not really. I said *also*. I liked the test you had with a and b declared
before the method, and was asking for further testing on some c and d declared
after the method.

http://codereview.appspot.com/4975049/diff/14001/lib/Parse/ParseDecl.cpp
File lib/Parse/ParseDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/14001/lib/Parse/ParseDecl.cpp#newc...
lib/Parse/ParseDecl.cpp:738: for (unsigned I = 0, NI =
Class.LateParsedDeclarations.size(); I < NI; ++I) {
We commonly use "I" to indicate an iterator. 'i' and 'e' or 'i' and 'size' are
the idiomatic names for integer indexes.

Yes, I realize that Clang is particularly bad at following this rule, but I'd
rather make it better than worse here.

http://codereview.appspot.com/4975049/diff/14001/lib/Parse/ParseDeclCXX.cpp
File lib/Parse/ParseDeclCXX.cpp (right):

http://codereview.appspot.com/4975049/diff/14001/lib/Parse/ParseDeclCXX.cpp#n...
lib/Parse/ParseDeclCXX.cpp:1787: for (unsigned I = 0, NI =
LateParsedAttrs.size(); I < NI; ++I) {
Same comment about the loop.

[delesley, 2011-09-07 19:13:27]

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/CMakeList...
File include/clang/Parse/CMakeLists.txt (right):

http://codereview.appspot.com/4975049/diff/2002/include/clang/Parse/CMakeList...
include/clang/Parse/CMakeLists.txt:5: DEPENDS AttrLateParsed.inc)
On 2011/09/02 18:45:30, chandlerc wrote:
> On 2011/09/02 18:19:08, delesley wrote:
> > This is based off the clang_tablegen rule for AttrSpellings, which does have
> the
> > line.
> 
> That is literally the only use clang_tablegen which has this. If you read the
> actual documentation for clang_tablegen, it clearly lists this field as for
> "extra-dependencies". I'll fix AttrSpellings in a bit.

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp
File lib/Parse/ParseDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:61: static bool isAttributeLateParsed(const
IdentifierInfo *II) {
On 2011/09/02 18:45:30, chandlerc wrote:
> On 2011/09/02 18:19:08, delesley wrote:
> > Adapted from HasAttribute function.
> 
> Ok, but would a const reference make more sense?

Done.

http://codereview.appspot.com/4975049/diff/2002/lib/Parse/ParseDecl.cpp#newco...
lib/Parse/ParseDecl.cpp:768: assert(LA.D);
On 2011/09/02 18:45:30, chandlerc wrote:
> On 2011/09/02 18:19:08, delesley wrote:
> > On 2011/09/01 22:45:45, chandlerc wrote:
> > > add a message for why this should never happen?
> > 
> > Done.
> 
> In general, it helps to condense the message into a string that the assert
> prints out. I was trying to suggest something like:
> 
> assert(LA.D && "No decl attached to late parsed attribute");

Done.

http://codereview.appspot.com/4975049/diff/2002/test/SemaCXX/warn-thread-safe...
File test/SemaCXX/warn-thread-safety-analysis.cpp (right):

http://codereview.appspot.com/4975049/diff/2002/test/SemaCXX/warn-thread-safe...
test/SemaCXX/warn-thread-safety-analysis.cpp:426: }
On 2011/09/02 18:45:30, chandlerc wrote:
> On 2011/09/02 18:19:08, delesley wrote:
> > On 2011/09/01 22:45:45, chandlerc wrote:
> > > It would be good to also test warnings on constructs here that require
> > > attributes that only appear later in the class definition.
> > 
> > Done.
> 
> Er, not really. I said *also*. I liked the test you had with a and b declared
> before the method, and was asking for further testing on some c and d declared
> after the method.

Done.

http://codereview.appspot.com/4975049/diff/14001/lib/Parse/ParseDecl.cpp
File lib/Parse/ParseDecl.cpp (right):

http://codereview.appspot.com/4975049/diff/14001/lib/Parse/ParseDecl.cpp#newc...
lib/Parse/ParseDecl.cpp:738: for (unsigned I = 0, NI =
Class.LateParsedDeclarations.size(); I < NI; ++I) {
On 2011/09/02 18:45:30, chandlerc wrote:
> We commonly use "I" to indicate an iterator. 'i' and 'e' or 'i' and 'size' are
> the idiomatic names for integer indexes.
> 
> Yes, I realize that Clang is particularly bad at following this rule, but I'd
> rather make it better than worse here.

Done.

http://codereview.appspot.com/4975049/diff/14001/lib/Parse/ParseDeclCXX.cpp
File lib/Parse/ParseDeclCXX.cpp (right):

http://codereview.appspot.com/4975049/diff/14001/lib/Parse/ParseDeclCXX.cpp#n...
lib/Parse/ParseDeclCXX.cpp:1787: for (unsigned I = 0, NI =
LateParsedAttrs.size(); I < NI; ++I) {
On 2011/09/02 18:45:30, chandlerc wrote:
> Same comment about the loop.

Done.