delesley added inline comments.
================
Comment at: lib/Analysis/ThreadSafety.cpp:951
+ }
} else {
+ // We're removing the underlying mutex. Warn on double unlocking.
----------------
aaronpuchert wrote:
> 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?
A scoped_lockable object is not a capability, it is an object that manages
capabilities. IMHO, conflating those two concepts is a bad idea, and recipe
for confusion. You would not, for example, use a scoped_lockable object in a
guarded_by attribute.
Unfortunately, the existing code tends to conflate "capabilities" and "facts",
which is my fault. A scoped_lockable object is a valid fact -- the fact
records whether the object is in scope -- it's just not a valid capability.
Simply renaming the methods from handleLock/Unlock to addFact/removeFact would
be a nice first step in making the code clearer, and would distinguish between
facts that refer to capabilities, and facts that refer to scoped objects.
================
Comment at: lib/Analysis/ThreadSafety.cpp:992
+ FSet.removeLock(FactMan, UnderCp);
+ FSet.addLock(FactMan, std::move(UnderEntry));
+ }
----------------
aaronpuchert wrote:
> 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.
That's not necessarily true. Not knowing whether a mutex is locked is
different from knowing that it is unlocked.
You're probably right in this case, because capabilities that are managed by
scoped objects obey a more restricted set of rules. But then, you're also
extending the ways in which scoped objects can be used. :-)
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
