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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
