aaronpuchert added a comment. Thanks for pointing out my error! Ignoring the implementation for a moment, do you think this is a good idea or will it produce to many false positives? Releasable/relockable scoped capabilities that I have seen keep track of the status, so it makes sense, but maybe there are others out there.
================ Comment at: lib/Analysis/ThreadSafety.cpp:929 + return Handler.handleDoubleLock(DiagKind, entry.toString(), entry.loc()); + Locked = true; + ---------------- delesley wrote: > It's been a while since I last looked at this code, but I don't think you can > use mutable fields in a FactEntry. The analysis creates a FactSet for each > program point, but each FactSet simply has pointers (FactIDs) for the > underlying FactEntries. If you change the definition of a FactEntry, it will > change that definition for every program point. So if you do an unlock on > one side of a branch, and change the underlying FactEntry to reflect that, > then analysis will then think you have also done the unlock on the other side > of the branch. > > Any changes should always be done by adding or removing entries from the > FactSet, not by mutating the underlying FactEntries. > That explains why there are problems with back edges. Now that you say it I'm actually aware of how the `FactManager` and `FactSet` work, but apparently I didn't consider that here. Would you mind if I make a separate change that returns only `const FactEntry*` from the `FactManager`? Maybe that makes it harder to write wrong code in the future. Repository: rC Clang https://reviews.llvm.org/D51187 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
