|
|
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fixes based on reviewer feedback #
Total messages: 3
|
chandlerc
http://codereview.appspot.com/4954041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4954041/diff/1/include/clang/Basic/DiagnosticSemaKinds.td#newcode1411 include/clang/Basic/DiagnosticSemaKinds.td:1411: "accessing dereferenced variable '%0' requires lock '%1'">, I wonder ...
|
14 years, 4 months ago (2011年08月29日 17:52:41 UTC) #1 | ||||||||||||||||||||||||||||||||
http://codereview.appspot.com/4954041/diff/1/include/clang/Basic/DiagnosticSe... File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4954041/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1411: "accessing dereferenced variable '%0' requires lock '%1'">, I wonder if this would be read better as: accessing the value pointed to by variable '...' requires lock '...' http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (left): http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:823: /// \param LockExp The lock expression corresponding to the lock to be added Why did this comment leave? http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:855: if (DR) Always put these inside the condition: if (const DeclRefExpr *DR = dyn_cast<...>(...)) return DR->getDecl(); http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:862: return NULL; s/NULL/0/ http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:867: /// \param Exp An expression that has been read or written. Parameter commets shouldn't be placed in the middle of the comment block. This entire comment is really hard to read. It could probably be cleaned up some to flow better and/or be formatted better.
Made these changes. Ready to commit? http://codereview.appspot.com/4954041/diff/1/include/clang/Basic/DiagnosticSe... File include/clang/Basic/DiagnosticSemaKinds.td (right): http://codereview.appspot.com/4954041/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1411: "accessing dereferenced variable '%0' requires lock '%1'">, On 2011年08月29日 17:52:41, chandlerc wrote: > I wonder if this would be read better as: > > accessing the value pointed to by variable '...' requires lock '...' Changed to: accessing the value pointed to by '...' requires lock '...' http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (left): http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:823: /// \param LockExp The lock expression corresponding to the lock to be added On 2011年08月29日 17:52:41, chandlerc wrote: > Why did this comment leave? Comment is back! http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:855: if (DR) On 2011年08月29日 17:52:41, chandlerc wrote: > Always put these inside the condition: > > if (const DeclRefExpr *DR = dyn_cast<...>(...)) > return DR->getDecl(); Done. http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:862: return NULL; On 2011年08月29日 17:52:41, chandlerc wrote: > s/NULL/0/ Done. http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:867: /// \param Exp An expression that has been read or written. On 2011年08月29日 17:52:41, chandlerc wrote: > Parameter commets shouldn't be placed in the middle of the comment block. > > This entire comment is really hard to read. It could probably be cleaned up some > to flow better and/or be formatted better. I deleted the "However" block and re-arranged the comment.
LGTM (modulo the comment below) http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.cpp File lib/Sema/AnalysisBasedWarnings.cpp (right): http://codereview.appspot.com/4954041/diff/1/lib/Sema/AnalysisBasedWarnings.c... lib/Sema/AnalysisBasedWarnings.cpp:867: /// \param Exp An expression that has been read or written. On 2011年08月29日 18:40:51, supertri wrote: > On 2011年08月29日 17:52:41, chandlerc wrote: > > Parameter commets shouldn't be placed in the middle of the comment block. > > > > This entire comment is really hard to read. It could probably be cleaned up > some > > to flow better and/or be formatted better. > > I deleted the "However" block and re-arranged the comment. I think we do need to document the limitations of this approach as currently implemented.