aaronpuchert added a comment.
Maybe you should have a look at the tests first. I thought about the semantics
that I think you are suggesting, but then we could have code like:
class SCOPED_LOCKABLE MutexLockUnlock {
public:
MutexLockUnlock(Mutex *mu1, Mutex *mu2) EXCLUSIVE_UNLOCK_FUNCTION(mu1)
EXCLUSIVE_LOCK_FUNCTION(mu2);
~MutexLockUnlock() /* what annotation do we use here? */;
void a() EXCLUSIVE_UNLOCK_FUNCTION();
void b() EXCLUSIVE_LOCK_FUNCTION();
};
Then what do `a` and `b` do? Or do we not allow this pattern? It's not
straightforward either way, which is why I wanted to talk my way out of this in
the bug report. ;)
================
Comment at: lib/Analysis/ThreadSafety.cpp:893
private:
- SmallVector<const til::SExpr *, 4> UnderlyingMutexes;
+ enum UnderlyingCapabilityKind {
+ UCK_Acquired, ///< Any kind of acquired capability.
----------------
delesley wrote:
> IMHO, it would make more sense to break this into two properties (or bits):
> acquired/released and shared/exclusive.
We don't use the information (shared/exclusive) for acquired locks, but we need
two bits anyway, so why not.
================
Comment at: lib/Analysis/ThreadSafety.cpp:916
+ for (const auto &M : ShrdRel)
+ UnderlyingMutexes.emplace_back(M.sexpr(), UCK_ReleasedExclusive);
}
----------------
delesley wrote:
> UCK_ReleasedShared? (Shouldn't this have been caught by a test case?)
There is no test case for the shared variant yet. I'll add one.
================
Comment at: lib/Analysis/ThreadSafety.cpp:951
+ }
} else {
+ // We're removing the underlying mutex. Warn on double unlocking.
----------------
delesley wrote:
> 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.
I agree that it's confusing, but it follows what I think was the idea behind
scoped capabilities: that they are also capabilities that can be
acquired/released. Now since the scoped capability releases a mutex on
construction (i.e. when it is acquired), it has to acquire the mutex when
released. So the way it handles the released mutexes mirrors what happens on
the scoped capability itself.
It's definitely not very intuitive, but I feel it's the most consistent variant
with what we have already.
The nice thing about this is that it works pretty well with the existing
semantics and allows constructs such as `MutexLockUnlock` (see the tests) that
unlocks one mutex and locks another. Not sure if anyone needs this, but why not?
================
Comment at: lib/Analysis/ThreadSafety.cpp:992
+ FSet.removeLock(FactMan, UnderCp);
+ FSet.addLock(FactMan, std::move(UnderEntry));
+ }
----------------
delesley wrote:
> Shouldn't this be outside of the else?
If we don't find `UnderCp` anymore, the negation should be there already. But I
can also keep it outside if you like.
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
