[cfe-commits] r139368 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/AnalysisBasedWarnings.cpp test/SemaCXX/warn-thread-safety-analysis.cpp

[Caitlin Sadowski]

Updated version.

Cheers,

Caitlin

On Mon, Sep 12, 2011 at 4:25 PM, Chandler Carruth <chandlerc at google.com> wrote:
> On Mon, Sep 12, 2011 at 3:57 PM, Caitlin Sadowski <supertri at google.com>
> 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 <chandlerc at google.com>
>> wrote:
>> > On Fri, Sep 9, 2011 at 9:07 AM, Caitlin Sadowski <supertri at google.com>
>> > 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.
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: threadsafety_guardedvar2.patch
Type: text/x-patch
Size: 5622 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110913/f5ce8648/attachment.bin>