|
|
Patch Set 1 #
Total comments: 10
Total messages: 2
|
chandlerc
LGTM w/ the comments below... http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/ThreadSafety.h File include/clang/Analysis/Analyses/ThreadSafety.h (right): http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/ThreadSafety.h#newcode63 include/clang/Analysis/Analyses/ThreadSafety.h:63: /// \enum SK_LockedSomePredecessors -- ...
|
14 years, 3 months ago (2011年09月15日 00:40:53 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||
LGTM w/ the comments below... http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... File include/clang/Analysis/Analyses/ThreadSafety.h (right): http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... include/clang/Analysis/Analyses/ThreadSafety.h:63: /// \enum SK_LockedSomePredecessors -- a mutex is locked in some but not all I wonder if there is a less technical description here... LockedSomePaths? Also, rather than "Some" maybe we want a better word. We want a word that more clearly is strictly greater than zero, and strictly less than all... Not coming up with anything better than "some" though... http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... include/clang/Analysis/Analyses/ThreadSafety.h:66: /// function. Same enum comment comment as the other patch... http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... include/clang/Analysis/Analyses/ThreadSafety.h:99: /// 2, or a mutex is only held at the start of some loop iterations, Typically formal lists place the conjunction on the prior item: 1. Foo is true, or 2. Bar is true, or 3. Baz is true. http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... include/clang/Analysis/Analyses/ThreadSafety.h:104: /// \param LEK -- which of the three above cases we should warn for ErrorKind? http://codereview.appspot.com/4964080/diff/1/include/clang/Basic/DiagnosticSe... File include/clang/Basic/DiagnosticSemaKinds.td (left): http://codereview.appspot.com/4964080/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1398: "mutex '%0' is still held at the end of function '%1'">, I'm sad to see this bit of information leave the diagnostic... can we get it back?
http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... File include/clang/Analysis/Analyses/ThreadSafety.h (right): http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... include/clang/Analysis/Analyses/ThreadSafety.h:63: /// \enum SK_LockedSomePredecessors -- a mutex is locked in some but not all On 2011年09月15日 00:40:53, chandlerc wrote: > I wonder if there is a less technical description here... LockedSomePaths? > > Also, rather than "Some" maybe we want a better word. We want a word that more > clearly is strictly greater than zero, and strictly less than all... Not coming > up with anything better than "some" though... Also could not come up with anything better (and neither could the person sitting next to me on the bus). Changed to LockedSomePaths. http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... include/clang/Analysis/Analyses/ThreadSafety.h:66: /// function. On 2011年09月15日 00:40:53, chandlerc wrote: > Same enum comment comment as the other patch... Done. http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... include/clang/Analysis/Analyses/ThreadSafety.h:99: /// 2, or a mutex is only held at the start of some loop iterations, On 2011年09月15日 00:40:53, chandlerc wrote: > Typically formal lists place the conjunction on the prior item: > > 1. Foo is true, or > 2. Bar is true, or > 3. Baz is true. Yes, yes they do. http://codereview.appspot.com/4964080/diff/1/include/clang/Analysis/Analyses/... include/clang/Analysis/Analyses/ThreadSafety.h:104: /// \param LEK -- which of the three above cases we should warn for On 2011年09月15日 00:40:53, chandlerc wrote: > ErrorKind? Done. http://codereview.appspot.com/4964080/diff/1/include/clang/Basic/DiagnosticSe... File include/clang/Basic/DiagnosticSemaKinds.td (left): http://codereview.appspot.com/4964080/diff/1/include/clang/Basic/DiagnosticSe... include/clang/Basic/DiagnosticSemaKinds.td:1398: "mutex '%0' is still held at the end of function '%1'">, On 2011年09月15日 00:40:53, chandlerc wrote: > I'm sad to see this bit of information leave the diagnostic... can we get it > back? Added a FIXME for now. It may be easier to put back after subsequent refactoring changes. Once DeLesley has finished implementing code to deal with all gcc test cases, you guys should think carefully about the error message text.