[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Delesley Hutchins via Phabricator via cfe-commits]

delesley added inline comments.


================
Comment at: lib/Analysis/ThreadSafety.cpp:893
 private:
-  SmallVector<const til::SExpr *, 4> UnderlyingMutexes;
+  enum UnderlyingCapabilityKind {
+    UCK_Acquired,          ///< Any kind of acquired capability.
----------------
IMHO, it would make more sense to break this into two properties (or bits): acquired/released and shared/exclusive. 


================
Comment at: lib/Analysis/ThreadSafety.cpp:916
+    for (const auto &M : ShrdRel)
+      UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
   }
----------------
UCK_ReleasedShared?  (Shouldn't this have been caught by a test case?)


================
Comment at: lib/Analysis/ThreadSafety.cpp:926
+          FactMan, CapabilityExpr(UnderlyingMutex.getPointer(), false));
+      if ((UnderlyingMutex.getInt() == UCK_Acquired) == (Entry != nullptr)) {
         // If this scoped lock manages another mutex, and if the underlying
----------------
This if statement makes my head hurt.  Can you find a different way of expressing it?


================
Comment at: lib/Analysis/ThreadSafety.cpp:951
+        }
       } else {
+        // We're removing the underlying mutex. Warn on double unlocking.
----------------
I find this very confusing.  A lock here unlocks the underlying mutex, and vice versa.  At the very least, some methods need to be renamed, or maybe we can have separate classes for ScopedLockable and ScopedUnlockable. 


================
Comment at: lib/Analysis/ThreadSafety.cpp:992
+            FSet.removeLock(FactMan, UnderCp);
+            FSet.addLock(FactMan, std::move(UnderEntry));
+          }
----------------
Shouldn't this be outside of the else?


Repository:
  rC Clang

https://reviews.llvm.org/D52578