Updated version. Cheers,
Caitlin On Mon, Sep 12, 2011 at 4:25 PM, Chandler Carruth <[email protected]> wrote: > On Mon, Sep 12, 2011 at 3:57 PM, Caitlin Sadowski <[email protected]> > wrote: >> >> Here is patch that goes back to the old separate diagnostics. > > Generally fine, but one nit-pick: > diff --git a/lib/Sema/AnalysisBasedWarnings.cpp > b/lib/Sema/AnalysisBasedWarnings.cpp > index 6a7c5d3..37f4ffc 100644 > --- a/lib/Sema/AnalysisBasedWarnings.cpp > +++ b/lib/Sema/AnalysisBasedWarnings.cpp > @@ -689,9 +689,21 @@ class ThreadSafetyReporter : public > clang::thread_safety::ThreadSafetyHandler { > > void handleNoMutexHeld(const NamedDecl *D, ProtectedOperationKind POK, > AccessKind AK, SourceLocation Loc) { > - // FIXME: It would be nice if this case printed without single quotes > around > - // the phrase 'any mutex' > - handleMutexNotHeld(D, POK, "any mutex", getLockKindFromAccessKind(AK), > Loc); > + assert(POK != POK_FunctionCall); > + unsigned DiagID; > + switch (POK) { > After the assert above, a switch seems heavy handed. If there are only ever > likely to be two candidates, then assert the *positive* cases and then use a > ternary: > assert((POK == POK_VarAccess || POK == POK_VarDereference) && > "..."); > unsigned DiagID = POK == POK_VarAccess? ... : ...; > If there are likely to be more than 2, then I would make the switch actually > cover all cases and 'assert(0 && ...)' on each case that cannot happen. > Switch + default == bad. =] > + case POK_VarAccess: > + DiagID = diag::warn_variable_requires_any_lock; > + break; > + case POK_VarDereference: > + DiagID = diag::warn_var_deref_requires_any_lock; > + break; > + default: > + break; > + } > + PartialDiagnostic Warning = S.PDiag(DiagID) > + << D->getName() << getLockKindFromAccessKind(AK); > + Warnings.push_back(DelayedDiag(Loc, Warning)); > } > >> >> Cheers, >> >> Caitlin >> >> On Fri, Sep 9, 2011 at 9:13 AM, Chandler Carruth <[email protected]> >> wrote: >> > On Fri, Sep 9, 2011 at 9:07 AM, Caitlin Sadowski <[email protected]> >> > wrote: >> >> >> >> + // FIXME: It would be nice if this case printed without single >> >> quotes >> >> around >> >> + // the phrase 'any mutex' >> > >> > I think you should use two diagnostics, or use a %select in the message. >> > The >> > C++ code shouldn't be providing "any mutex", all the text should come >> > from >> > the diagnostic message table. > >
From fd1330e799176f76d65ca7c652710b632abd5778 Mon Sep 17 00:00:00 2001 From: Caitlin Sadowski <[email protected]> Date: Mon, 12 Sep 2011 15:53:21 -0700 Subject: [PATCH] Thread safety: reverting to use separate warning for requirement to hold any lock --- include/clang/Basic/DiagnosticSemaKinds.td | 8 ++++++++ lib/Sema/AnalysisBasedWarnings.cpp | 13 +++++++++---- test/SemaCXX/warn-thread-safety-analysis.cpp | 16 ++++++++-------- 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td index 0a0fea3..61e2d37 100644 --- a/include/clang/Basic/DiagnosticSemaKinds.td +++ b/include/clang/Basic/DiagnosticSemaKinds.td @@ -1418,6 +1418,14 @@ def warn_var_deref_requires_lock : Warning< "%select{reading|writing}2 the value pointed to by '%0' requires lock on '%1'" " to be %select{held|held exclusively}2">, InGroup<ThreadSafety>, DefaultIgnore; +def warn_variable_requires_any_lock : Warning< + "%select{reading|writing}1 variable '%0' requires lock on any mutex to be " + "%select{held|held exclusively}1">, + InGroup<ThreadSafety>, DefaultIgnore; +def warn_var_deref_requires_any_lock : Warning< + "%select{reading|writing}1 the value pointed to by '%0' requires lock on any" + " mutex to be %select{held|held exclusively}1">, + InGroup<ThreadSafety>, DefaultIgnore; def warn_fun_requires_lock : Warning< "calling function '%0' requires %select{shared|exclusive}2 lock on '%1'">, InGroup<ThreadSafety>, DefaultIgnore; diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp index 8ea8a67..bbd8fa3 100644 --- a/lib/Sema/AnalysisBasedWarnings.cpp +++ b/lib/Sema/AnalysisBasedWarnings.cpp @@ -660,9 +660,14 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler { void handleNoMutexHeld(const NamedDecl *D, ProtectedOperationKind POK, AccessKind AK, SourceLocation Loc) { - // FIXME: It would be nice if this case printed without single quotes around - // the phrase 'any mutex' - handleMutexNotHeld(D, POK, "any mutex", getLockKindFromAccessKind(AK), Loc); + assert((POK == POK_VarAccess || POK == POK_VarDereference) + && "Only works for variables"); + unsigned DiagID = POK == POK_VarAccess? + diag::warn_variable_requires_any_lock: + diag::warn_var_deref_requires_any_lock; + PartialDiagnostic Warning = S.PDiag(DiagID) + << D->getName() << getLockKindFromAccessKind(AK); + Warnings.push_back(DelayedDiag(Loc, Warning)); } void handleMutexNotHeld(const NamedDecl *D, ProtectedOperationKind POK, @@ -680,7 +685,7 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler { break; } PartialDiagnostic Warning = S.PDiag(DiagID) - << D->getName().str() << LockName << LK; + << D->getName() << LockName << LK; Warnings.push_back(DelayedDiag(Loc, Warning)); } diff --git a/test/SemaCXX/warn-thread-safety-analysis.cpp b/test/SemaCXX/warn-thread-safety-analysis.cpp index f9ba5ec..899076b 100644 --- a/test/SemaCXX/warn-thread-safety-analysis.cpp +++ b/test/SemaCXX/warn-thread-safety-analysis.cpp @@ -351,12 +351,12 @@ void gb_fun_3() { void gb_bad_0() { sls_guard_var = 1; // \ - // expected-warning{{writing variable 'sls_guard_var' requires lock on 'any mutex' to be held exclusively}} + // expected-warning{{writing variable 'sls_guard_var' requires lock on any mutex to be held exclusively}} } void gb_bad_1() { int x = sls_guard_var; // \ - // expected-warning{{reading variable 'sls_guard_var' requires lock on 'any mutex' to be held}} + // expected-warning{{reading variable 'sls_guard_var' requires lock on any mutex to be held}} } void gb_bad_2() { @@ -371,12 +371,12 @@ void gb_bad_3() { void gb_bad_4() { *pgb_gvar = 1; // \ - // expected-warning {{writing the value pointed to by 'pgb_gvar' requires lock on 'any mutex' to be held exclusively}} + // expected-warning {{writing the value pointed to by 'pgb_gvar' requires lock on any mutex to be held exclusively}} } void gb_bad_5() { int x = *pgb_gvar; // \ - // expected-warning {{reading the value pointed to by 'pgb_gvar' requires lock on 'any mutex' to be held}} + // expected-warning {{reading the value pointed to by 'pgb_gvar' requires lock on any mutex to be held}} } void gb_bad_6() { @@ -397,13 +397,13 @@ void gb_bad_8() { void gb_bad_9() { sls_guard_var++; // \ - // expected-warning{{writing variable 'sls_guard_var' requires lock on 'any mutex' to be held exclusively}} + // expected-warning{{writing variable 'sls_guard_var' requires lock on any mutex to be held exclusively}} sls_guard_var--; // \ - // expected-warning{{writing variable 'sls_guard_var' requires lock on 'any mutex' to be held exclusively}} + // expected-warning{{writing variable 'sls_guard_var' requires lock on any mutex to be held exclusively}} ++sls_guard_var; // \ - // expected-warning{{writing variable 'sls_guard_var' requires lock on 'any mutex' to be held exclusively}} + // expected-warning{{writing variable 'sls_guard_var' requires lock on any mutex to be held exclusively}} --sls_guard_var;// \ - // expected-warning{{writing variable 'sls_guard_var' requires lock on 'any mutex' to be held exclusively}} + // expected-warning{{writing variable 'sls_guard_var' requires lock on any mutex to be held exclusively}} } //-----------------------------------------------// -- 1.7.3.1
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
