Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1292)

Issue 4707044: Finished basic parsing for all attributes (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 8 months ago by supertri
Modified:
12 years, 6 months ago
Reviewers:
chandlerc, scshunt
CC:
cymbal-team_google.com, delesley
Visibility:
Public.

Patch Set 1 #

Total comments: 1

Patch Set 2 : Adding documentation #

Total comments: 40
Unified diffs Side-by-side diffs Delta from patch set Stats (+1015 lines, -19 lines) Patch
M docs/LanguageExtensions.html View 1 3 chunks +133 lines, -16 lines 10 comments Download
M include/clang/Basic/Attr.td View 1 1 chunk +52 lines, -0 lines 0 comments Download
M include/clang/Basic/DiagnosticSemaKinds.td View 1 1 chunk +2 lines, -0 lines 0 comments Download
M include/clang/Sema/AttributeList.h View 1 5 chunks +13 lines, -0 lines 0 comments Download
M lib/Sema/AttributeList.cpp View 1 1 chunk +13 lines, -0 lines 0 comments Download
M lib/Sema/SemaDeclAttr.cpp View 1 7 chunks +227 lines, -3 lines 24 comments Download
M test/SemaCXX/warn-thread-safety.cpp View 2 chunks +575 lines, -0 lines 6 comments Download

Messages

Total messages: 3
scshunt
Just one comment there. Otherwise looks good. http://codereview.appspot.com/4707044/diff/1/lib/Sema/SemaDeclAttr.cpp File lib/Sema/SemaDeclAttr.cpp (right): http://codereview.appspot.com/4707044/diff/1/lib/Sema/SemaDeclAttr.cpp#newcode211 lib/Sema/SemaDeclAttr.cpp:211: /// \return ...
12 years, 8 months ago (2011-07-20 01:57:59 UTC) #1
chandlerc
This is the right version of the patch for me to be reviewing? Is there ...
12 years, 8 months ago (2011-08-01 18:23:52 UTC) #2
supertri
12 years, 8 months ago (2011-08-01 21:03:39 UTC) #3
http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html
File docs/LanguageExtensions.html (right):

http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#...
docs/LanguageExtensions.html:1125: <h4
id="ts_noanal">no_thread_safety_analysis</h4>
On 2011/08/01 18:23:53, chandlerc wrote:
> given a namespace, would 'skip_analysis' be a better name?

Are we for-sure transitioning to a namespace? Or is this still under discussion?
When I initially floated the idea a month or so ago you convinced me to not use
a namespace for now...

http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#...
docs/LanguageExtensions.html:1159: specify that the variable must be accessed
while holding lock l.</p>
Fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> <tt>l</tt> here and below.

http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#...
docs/LanguageExtensions.html:1164: specify that the pointer must be dereferenced
while holding lock l.</p>
It is the latter. To guard the pointer, you annotate with guarded_by. However,
the current gcc impl does not do any fancy pointer analysis -- annotating a
pointer with pt_guarded_by(z) just means that at the time the pointer is
dereferenced you must be holding z. 

On 2011/08/01 18:23:53, chandlerc wrote:
> This implies that the above attribute needs some important clarifications:
when
> annotating a pointer, is it the pointer that is guarded or the
pointed-to-value?
> 
> I would intuitively expect the former, but this attribute makes me feel it is
in
> fact the latter! Is this really the way these today work??

http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#...
docs/LanguageExtensions.html:1185: arguments: either of lockable type or
integers indexing into
Yes. I think the implementation of annotalysis does also allow using the
function parameters explicitly as arguments (which are arguments of lockable
type). 

On 2011/08/01 18:23:53, chandlerc wrote:
> Do we want to hint at more user-friendly options than function parameter
> indexing with integers? Specifically that we plan to add attributes directly
on
> the function parameters when that is well defined (C++0x attributes)?

http://codereview.appspot.com/4707044/diff/4001/docs/LanguageExtensions.html#...
docs/LanguageExtensions.html:1193: the locks may be shared (e.g. read locks).
Fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> From here on down I feel like several of the paragraphs need to be
re-flowed...

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp
File lib/Sema/SemaDeclAttr.cpp (right):

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:30: enum AttributeDeclType {
I just went back to an anonymous name.

On 2011/08/01 18:23:53, chandlerc wrote:
> This should be a '...Kind' rather than '...Type'.

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:301: bool pointer = false) {
fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> indent is off

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:381: bool exclusive = false) {
Fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> indent

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:384: // zero or more arguments ok
Each function for handling an attribute has similar structure, starting with
checking to make sure the number of arguments matches. This does not need such a
check since zero or more args are ok. So this is documenting that fact so that
it is clear the num args check was not just forgotten.

On 2011/08/01 18:23:53, chandlerc wrote:
> ? Not sure why this is here

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:415: S.Context));
fixed

On 2011/08/01 18:23:53, chandlerc wrote:
> indent

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:423: bool exclusive = false) {
fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> indent

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:437: S.Context));
fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> indent

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:440: S.Context));
fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> indent

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:445: const AttributeList &Attr) {
fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> indent

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:448: // zero or more arguments ok
I would prefer to keep it. See above discussion.

On 2011/08/01 18:23:53, chandlerc wrote:
> No need for comment.

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:460: const AttributeList &Attr) {
fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> indent

http://codereview.appspot.com/4707044/diff/4001/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:476: const AttributeList &Attr) {
fixed
On 2011/08/01 18:23:53, chandlerc wrote:
> indent

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

http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe...
test/SemaCXX/warn-thread-safety.cpp:6: */
Changed to //-----------------------------------------//
banner system.

On 2011/08/01 18:23:53, chandlerc wrote:
> Why do we have a new comment system here? These are tests. We don't need this
> much documentation.

http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe...
test/SemaCXX/warn-thread-safety.cpp:16: ***********************************/
Changed to //-----------------------------------------//
banner system.

They make it much easier for me to browse/edit the test file. 

On 2011/08/01 18:23:53, chandlerc wrote:
> If you're going to use banner comments, use an existing banner comment pattern
> in LLVM/Clang.
> 
> That said, I don't think they add anything.

http://codereview.appspot.com/4707044/diff/4001/test/SemaCXX/warn-thread-safe...
test/SemaCXX/warn-thread-safety.cpp:261: //2.Deal with argument parsing:
Already replaced by the next patch. 

On 2011/08/01 18:23:53, chandlerc wrote:
> This should either not be here, or have a FIXME attached and likely use #if 0
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b