delesley added a comment. I have a few concerns. First, this patch introduces an inconsistency between managed and unmanaged locks. For unmanaged locks, we warn, and //then assume the lock is held exclusively//. For managed locks, we don't warn, but //assume it is held shared//. The warn/not-warn is consistent with more relaxed handling of managed locks, but exclusive/shared difference could lead to confusion. Both mechanisms are sound; they're just not consistent. Any thoughts?
Second, the mixing of AssertHeld() and Lock() on different control flow branches looks very weird to me. I can see one obvious case where you might want to do that, e.g. if (mu_.is_locked()) mu_.AssertHeld(); else mu_.Lock(). However, if a function locks mu_ on one branch, and assert mu_ on the other, then that usually indicates a problem. It means that the lock-state going into that function was unknown; the programmer didn't know whether mu_ was locked or not, and that's never good. The //only// valid condition in that case is to check whether mu_ is locked -- any other condition would be unsound. The correct way to deal with this situation is to mark the is_locked() method with an attribute, e.g. TEST_LOCKED_FUNCTION, rather than relying on the AssertHeld mechanism. That's a lot more work, but I would prefer to at least have a TODO comment indicating that the AssertHeld is a workaround, and restrict the mixing of assert/lock to managed locks in the mean time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102026/new/ https://reviews.llvm.org/D102026 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
