Issue 4804041: Basic argument parsing for all the attributes.

Issue 4804041: Basic argument parsing for all the attributes. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 9 months ago by supertri

Modified:
12 years, 7 months ago

Reviewers:
chandlerc, scshunt

CC:
cymbal-team_google.com, delesley

Visibility:
Public.

Patch Set 2 : Some fixes based on comments #

Total comments: 2

Created: 12 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+25 lines, -19 lines)			Patch
M	include/clang/Basic/DiagnosticSemaKinds.td	View	1	1 chunk	+5 lines, -7 lines	2 comments	Download
M	lib/Parse/ParseDecl.cpp	View	1	1 chunk	+9 lines, -1 line	0 comments	Download
M	lib/Sema/SemaDeclAttr.cpp	View	1	1 chunk	+10 lines, -10 lines	0 comments	Download
M	test/SemaCXX/warn-thread-safety.cpp	View	1	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

scshunt

A few comments there. http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td#newcode1137 include/clang/Basic/DiagnosticSemaKinds.td:1137: "%0 attribute requires arguments that ...

12 years, 9 months ago (2011-07-20 02:21:39 UTC) #1

supertri

http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td#newcode1137 include/clang/Basic/DiagnosticSemaKinds.td:1137: "%0 attribute requires arguments that are objects or point ...

12 years, 8 months ago (2011-07-28 20:54:12 UTC) #2

http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSe...
File include/clang/Basic/DiagnosticSemaKinds.td (right):

http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSe...
include/clang/Basic/DiagnosticSemaKinds.td:1137: "%0 attribute requires
arguments that are objects or point to objects">;
I check if they are a RecordDecl. So I guess classes, structs, etc. What is a
better word?

On 2011/07/20 02:21:39, scshunt wrote:
> By "object", do you mean a class type here? If so, this should be more
specific
> - "object" is a bad word here. Probably better would be "requires arguments
that
> are of class type or pointers to class type" or something like that.

http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSe...
include/clang/Basic/DiagnosticSemaKinds.td:1309: "with 'lockable' attribute">,
I am divided. Lockable type is more concise, but mentioning the attribute
explicitly may make it easy to fix the error since it is more clear. Anyone else
have thoughts on this?

On 2011/07/20 02:21:39, scshunt wrote:
> I don't really like the explicit reference to the lockable attribute here - I
> would prefer something like "lockable type". Others may disagree though.

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

http://codereview.appspot.com/4804041/diff/1/lib/Parse/ParseDecl.cpp#newcode686
lib/Parse/ParseDecl.cpp:686: /// We need to special case the parsing due to the
fact that
:) Fixed. 

On 2011/07/20 02:21:39, scshunt wrote:
> This comment would do better with

http://codereview.appspot.com/4804041/diff/1/lib/Parse/ParseDecl.cpp#newcode693
lib/Parse/ParseDecl.cpp:693: ConsumeParen(); // ignore the left paren loc for
now
Probably not. This code is so far duplicated from the code in the main attribute
parsing loop for the case when the first argument is not an identifier so I do
not feel comfortable changing it here. 

On 2011/07/20 02:21:39, scshunt wrote:
> Is there no convenient location to store these on the attribute? If not, then
> ignore this. If so, then we should store there.

scshunt

Just one comment; other than that, LGTM http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td#newcode1137 include/clang/Basic/DiagnosticSemaKinds.td:1137: "%0 attribute ...

12 years, 8 months ago (2011-07-29 23:17:45 UTC) #3

chandlerc

http://codereview.appspot.com/4804041/diff/1/lib/Parse/ParseDecl.cpp File lib/Parse/ParseDecl.cpp (right): http://codereview.appspot.com/4804041/diff/1/lib/Parse/ParseDecl.cpp#newcode714 lib/Parse/ParseDecl.cpp:714: ConsumeParen(); // ignore the right paren loc for now ...

12 years, 8 months ago (2011-08-01 18:39:21 UTC) #4

supertri

12 years, 8 months ago (2011-08-01 21:40:37 UTC) #5

chandlerc

12 years, 8 months ago (2011-08-15 23:47:40 UTC) #6

I know it looses the comment threads, but can you create a fresh rietveld
issue, and move this there?

Currently, I can't get the actual planned diff that you're going to commit.

In particular, I think moving it and having a clean baseline is important
before involving upstream.

On Mon, Aug 1, 2011 at 2:40 PM, <supertri@google.com> wrote:

>
> http://codereview.appspot.com/**4804041/diff/1/include/clang/**
>
Basic/DiagnosticSemaKinds.td<http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td>
> File include/clang/Basic/**DiagnosticSemaKinds.td (right):
>
> http://codereview.appspot.com/**4804041/diff/1/include/clang/**
>
Basic/DiagnosticSemaKinds.td#**newcode1137<http://codereview.appspot.com/4804041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td#newcode1137>
> include/clang/Basic/**DiagnosticSemaKinds.td:1137: "%0 attribute requires
> arguments that are objects or point to objects">;
> Ok. Changed.
>
>
> On 2011/07/29 23:17:50, scshunt wrote:
>
>> On 2011/07/28 20:54:12, supertri wrote:
>> > I check if they are a RecordDecl. So I guess classes, structs, etc.
>>
> What is a
>
>> > better word?
>>
>
>  Class type would be a better word. All types except void and function
>>
> and
>
>> reference types are object types.
>>
>
>
http://codereview.appspot.com/**4804041/diff/1/lib/Parse/**ParseDecl.cpp<http...
> File lib/Parse/ParseDecl.cpp (right):
>
> http://codereview.appspot.com/**4804041/diff/1/lib/Parse/**
>
ParseDecl.cpp#newcode714<http://codereview.appspot.com/4804041/diff/1/lib/Parse/ParseDecl.cpp#newcode714>
> lib/Parse/ParseDecl.cpp:714: ConsumeParen(); // ignore the right paren
> loc for now
> Punting for now, as per discussion.
>
>
> On 2011/08/01 18:39:21, chandlerc wrote:
>
>> This might be worth a FIXME. We can often reconstruct the open paren
>>
> loc, but
>
>> not the close paren loc...
>>
>
>
http://codereview.appspot.com/**4804041/diff/1/lib/Sema/**SemaDeclAttr.cpp<ht...
> File lib/Sema/SemaDeclAttr.cpp (right):
>
> http://codereview.appspot.com/**4804041/diff/1/lib/Sema/**
>
SemaDeclAttr.cpp#newcode448<http://codereview.appspot.com/4804041/diff/1/lib/Sema/SemaDeclAttr.cpp#newcode448>
> lib/Sema/SemaDeclAttr.cpp:448: << Attr.getName();
> Fixed.
>
>
> On 2011/08/01 18:39:21, chandlerc wrote:
>
>> We don't return here?
>>
>
> http://codereview.appspot.com/**4804041/diff/5001/include/**clang/Basic/**
>
DiagnosticSemaKinds.td<http://codereview.appspot.com/4804041/diff/5001/include/clang/Basic/DiagnosticSemaKinds.td>
> File include/clang/Basic/**DiagnosticSemaKinds.td (left):
>
> http://codereview.appspot.com/**4804041/diff/5001/include/**clang/Basic/**
>
DiagnosticSemaKinds.td#**oldcode1317<http://codereview.appspot.com/4804041/diff/5001/include/clang/Basic/DiagnosticSemaKinds.td#oldcode1317>
> include/clang/Basic/**DiagnosticSemaKinds.td:1317:
> InGroup<DiagGroup<"thread-**safety">>;
> This section is added in the patch to be submitted.
>
>
> On 2011/08/01 18:39:21, chandlerc wrote:
>
>> These don't belong in this patch. Please submit these as a separate
>>
> cleanup
>
>> change. No need for pre-commit review there.
>>
>
>
http://codereview.appspot.com/**4804041/<http://codereview.appspot.com/4804041/>
>

Expand All Messages | Collapse All Messages