[PATCH] D49885: Thread safety analysis: Allow relockable scopes

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

The changes look reasonable to me (after fixing a few nits), but please give @delesley a chance to weigh in before committing.



================
Comment at: lib/Analysis/ThreadSafety.cpp:955
+      // We're relocking the underlying mutexes. Warn on double locking.
+      if (FSet.findLock(FactMan, UnderCp)) {
+        Handler.handleDoubleLock(DiagKind, UnderCp.toString(), RelockLoc);
----------------
Can elide the braces.


================
Comment at: lib/Analysis/ThreadSafety.cpp:960-961
+        FSet.removeLock(FactMan, !UnderCp);
+        FSet.addLock(FactMan, llvm::make_unique<LockableFactEntry>(UnderCp, LK,
+                                                                   RelockLoc));
+      }
----------------
aaronpuchert wrote:
> Looks a bit weird, but `clang-format` told me to do that.
This line looks correct to me, but the `else` statement above doesn't match our usual formatting rules, so I'm worried you're running clang-format in a way that's not picking up the LLVM coding style options.


================
Comment at: lib/Analysis/ThreadSafety.cpp:1326
+  // fact, it must be a ScopedLockableFactEntry.
+  auto SLDat = static_cast<const ScopedLockableFactEntry *>(LDat);
+  SLDat->Relock(FSet, FactMan, Kind, RelockLoc, Handler, DiagKind);
----------------
`auto *` to remind folks that it's a pointer.


Repository:
  rC Clang

https://reviews.llvm.org/D49885