<div class="gmail_quote">On Mon, Sep 12, 2011 at 3:57 PM, Caitlin Sadowski <span dir="ltr"><<a href="mailto:supertri@google.com">supertri@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Here is patch that goes back to the old separate diagnostics.<br></blockquote><div><br></div><div>Generally fine, but one nit-pick:</div><div><br></div><div><div>diff --git a/lib/Sema/AnalysisBasedWarnings.cpp b/lib/Sema/AnalysisBasedWarnings.cpp</div>
<div>index 6a7c5d3..37f4ffc 100644</div><div>--- a/lib/Sema/AnalysisBasedWarnings.cpp</div><div>+++ b/lib/Sema/AnalysisBasedWarnings.cpp</div><div>@@ -689,9 +689,21 @@ class ThreadSafetyReporter : public clang::thread_safety::ThreadSafetyHandler {</div>
<div> </div><div>   void handleNoMutexHeld(const NamedDecl *D, ProtectedOperationKind POK,</div><div>                          AccessKind AK, SourceLocation Loc) {</div><div>-    // FIXME: It would be nice if this case printed without single quotes around</div>
<div>-    // the phrase 'any mutex'</div><div>-    handleMutexNotHeld(D, POK, "any mutex", getLockKindFromAccessKind(AK), Loc);</div><div>+    assert(POK != POK_FunctionCall);</div><div>+    unsigned DiagID;</div>
<div>+    switch (POK) {</div><div><br></div><div>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:</div><div>
<br></div><div>assert((POK == POK_VarAccess || POK == POK_VarDereference) &&</div><div>       "...");</div><div>unsigned DiagID = POK == POK_VarAccess? ... : ...;</div><div><br></div><div>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. =]</div>
<div><br></div><div>+      case POK_VarAccess:</div><div>+        DiagID = diag::warn_variable_requires_any_lock;</div><div>+        break;</div><div>+      case POK_VarDereference:</div><div>+        DiagID = diag::warn_var_deref_requires_any_lock;</div>
<div>+        break;</div><div>+      default:</div><div>+        break;</div><div>+    }</div><div>+    PartialDiagnostic Warning = S.PDiag(DiagID)</div><div>+      << D->getName() << getLockKindFromAccessKind(AK);</div>
<div>+    Warnings.push_back(DelayedDiag(Loc, Warning));</div><div>   }</div></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<br>
Cheers,<br>
<span class="HOEnZb"><font color="#888888"><br>
Caitlin<br>
</font></span><div class="HOEnZb"><div></div><div class="h5"><br>
On Fri, Sep 9, 2011 at 9:13 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
> On Fri, Sep 9, 2011 at 9:07 AM, Caitlin Sadowski <<a href="mailto:supertri@google.com">supertri@google.com</a>><br>
> wrote:<br>
>><br>
>> +    // FIXME: It would be nice if this case printed without single quotes<br>
>> around<br>
>> +    // the phrase 'any mutex'<br>
><br>
> I think you should use two diagnostics, or use a %select in the message. The<br>
> C++ code shouldn't be providing "any mutex", all the text should come from<br>
> the diagnostic message table.<br>
</div></div></blockquote></div><br>