aaronpuchert added inline comments.
================ Comment at: clang/lib/Analysis/ThreadSafety.cpp:1644 if (!LDat) { - Analyzer->Handler.handleMutexNotHeld("", D, POK, Cp.toString(), - LK_Shared, Loc); + Analyzer->Handler.handleMutexNotHeld("negative capability", D, POK, + Cp.toString(), LK_Shared, Loc); ---------------- aaron.ballman wrote: > aaronpuchert wrote: > > aaron.ballman wrote: > > > It's a bit odd that we aren't using `DiagKind` as below, I assume that's > > > because this is a negative test and the others are positive tests, but > > > doesn't this introduce a terminology difference where a positive failure > > > may call it a mutex and a negative failure may call it a negative > > > capability? Should this be hooked in to `ClassifyDiagnostic()` (perhaps > > > we need a `ClassifyNegativeDiagnostic()`? > > My thinking was that whatever the positive capability is called, we should > > only talk about a "negative capability" instead of a "negative mutex" or a > > "negative role". Also because not holding a capability is in some way its > > own kind of capability. > I may still be confused or thinking of this differently, but I would assume > that a negative mutex would be a mutex that's explicitly not held, which you > may want to ensure on a function boundary to avoid deadlock. From that, I'd > have guessed we would want the diagnostic to read `cannot call function 'bar' > while mutex 'mu' is held` or `calling function 'bar' requires mutex 'mu' to > not be held` because that's more clear than talking about negative > capabilities (when the user is thinking in terms of mutexes which are or > aren't held). Now I get it. I don't see an issue with that, but we need to distinguish between `EXCLUDES(mu)` and `REQUIRES(!mu)`. The former will produce "cannot call function 'bar' while mutex 'mu' is held" and we probably want the latter to produce a different warning message. Now one argument for the existing scheme remains that with `-Wthread-safety-negative`, if you see a warning like "acquiring mutex 'mu' requires negative capability '!mu'" on a lock operation, you know you can fix that by adding `REQUIRES(!mu)` to your function. If we say "warning: acquiring mutex 'mu' requires mutex 'mu' not to be held" it might not be as clear what we want. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84603/new/ https://reviews.llvm.org/D84603 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits