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

Issue 4894048: Threadsafety: Initial lockset implementation (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 5 months ago by supertri
Modified:
10 years, 4 months ago
Reviewers:
chandlerc, delesley
Visibility:
Public.

Patch Set 1 #

Total comments: 16

Patch Set 2 : Updated patch set that uses PO iterators #

Total comments: 156

Patch Set 3 : Merging in reviewer edits along with switch to ImmutableMap #

Total comments: 28

Patch Set 4 : Merging in more reviewer edits: release candidate #

Total comments: 32
Unified diffs Side-by-side diffs Delta from patch set Stats (+974 lines, -44 lines) Patch
M include/clang/Basic/Attr.td View 2 chunks +14 lines, -0 lines 0 comments Download
M include/clang/Basic/DiagnosticSemaKinds.td View 1 2 1 chunk +18 lines, -1 line 0 comments Download
M include/clang/Sema/AnalysisBasedWarnings.h View 1 chunk +1 line, -0 lines 0 comments Download
M lib/Sema/AnalysisBasedWarnings.cpp View 1 2 3 6 chunks +567 lines, -0 lines 14 comments Download
M lib/Sema/SemaDeclAttr.cpp View 1 2 3 12 chunks +105 lines, -41 lines 18 comments Download
A test/SemaCXX/warn-thread-safety-analysis.cpp View 1 2 1 chunk +267 lines, -0 lines 0 comments Download
A + test/SemaCXX/warn-thread-safety-parsing.cpp View 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 8
chandlerc
Sorry these comments are so late in the process. I thought i had gone through ...
10 years, 5 months ago (2011-08-19 23:54:06 UTC) #1
supertri
I made various small edits throughout the patch. Let me know if I can commit ...
10 years, 5 months ago (2011-08-22 18:30:16 UTC) #2
delesley
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode604 lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet; It's supposed to implement an actual set ...
10 years, 5 months ago (2011-08-22 18:58:03 UTC) #3
chandlerc
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode650 lib/Sema/AnalysisBasedWarnings.cpp:650: namespace { //anonymous namespace for clang::thread_safety On 2011/08/22 18:30:16, ...
10 years, 5 months ago (2011-08-22 21:52:12 UTC) #4
delesley
http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/10001/lib/Sema/AnalysisBasedWarnings.cpp#newcode712 lib/Sema/AnalysisBasedWarnings.cpp:712: /// "x" is substituted for "this" so we get ...
10 years, 5 months ago (2011-08-22 22:02:52 UTC) #5
supertri
http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4894048/diff/2001/lib/Sema/AnalysisBasedWarnings.cpp#newcode604 lib/Sema/AnalysisBasedWarnings.cpp:604: llvm::BitVector BitSet; On 2011/08/22 18:58:03, delesley wrote: > It's ...
10 years, 5 months ago (2011-08-23 17:47:37 UTC) #6
chandlerc
Essentially cosmetic comments. Please re-write the description of whats going in to really carefully explain ...
10 years, 5 months ago (2011-08-23 18:10:04 UTC) #7
supertri
10 years, 5 months ago (2011-08-23 18:26:04 UTC) #8
http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning...
File lib/Sema/AnalysisBasedWarnings.cpp (right):

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning...
lib/Sema/AnalysisBasedWarnings.cpp:607: 
On 2011/08/23 18:10:04, chandlerc wrote:
> nuke blank line

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning...
lib/Sema/AnalysisBasedWarnings.cpp:666: /// \class Lock
On 2011/08/23 18:10:04, chandlerc wrote:
> Stale comment. You can actually delete this line, Doxygen should pick it up by
> looking below.

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning...
lib/Sema/AnalysisBasedWarnings.cpp:798: /// \class BuildLockset
On 2011/08/23 18:10:04, chandlerc wrote:
> Probably can remove this too

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning...
lib/Sema/AnalysisBasedWarnings.cpp:996: const Lockset LoopEntrySet,
On 2011/08/23 18:10:04, chandlerc wrote:
> indent is off

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning...
lib/Sema/AnalysisBasedWarnings.cpp:1063: // Create a default initial lockset in
case there are no predecessors.
On 2011/08/23 18:10:04, chandlerc wrote:
> This is no longer quite accurate... maybe just note that we already *have* a
> default initial lockset.

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning...
lib/Sema/AnalysisBasedWarnings.cpp:1099: for (CFGBlock::const_iterator BI =
CurrBlock->begin(), BE = CurrBlock->end();
On 2011/08/23 18:10:04, chandlerc wrote:
> 80 columns

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/AnalysisBasedWarning...
lib/Sema/AnalysisBasedWarnings.cpp:1143: 
On 2011/08/23 18:10:04, chandlerc wrote:
> Can probably nix 2 of the 4 blank lines.

Done.

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

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:275: /// resolves to a lockable object.
On 2011/08/23 18:10:04, chandlerc wrote:
> Join with the previous line.

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:279: //  Flag error if could not get record type for
this argument.
On 2011/08/23 18:10:04, chandlerc wrote:
> no 2 spaces after '//'

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:299: int Sidx = 0,
On 2011/08/23 18:10:04, chandlerc wrote:
> Sidx? Huh?

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:301: for(unsigned int Idx = Sidx; Idx <
Attr.getNumArgs(); ++Idx) {
On 2011/08/23 18:10:04, chandlerc wrote:
> s/unsigned int/unsigned/

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:307: QualType Arg_QT = ArgExp->getType();
On 2011/08/23 18:10:04, chandlerc wrote:
> Arg_QT -> ArgTy

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:309: // Get record type.
On 2011/08/23 18:10:04, chandlerc wrote:
> This doesn't seem to to add information reall...

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:313: // Now check if we idx into a record type
function param.
On 2011/08/23 18:10:04, chandlerc wrote:
> s/idx/index/

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:320: uint64_t ParamIdx_from1 =
ArgValue.getZExtValue();
On 2011/08/23 18:10:04, chandlerc wrote:
> ParamIdxFromOne?

Done.

http://codereview.appspot.com/4894048/diff/4002/lib/Sema/SemaDeclAttr.cpp#new...
lib/Sema/SemaDeclAttr.cpp:321: uint64_t ParamIdx_from0 = ParamIdx_from1 - 1;
On 2011/08/23 18:10:04, chandlerc wrote:
> ParamIdxFromZero?

Done.
Sign in to reply to this message.

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