On Tue, Sep 2, 2014 at 3:12 PM, Ed Schouten <[email protected]> wrote: > On 2 September 2014 20:56, Delesley Hutchins <[email protected]> wrote: >> The crash is probably being caused by the fact that in removeLock(), you are >> trying to remove the guard object multiple times. The call to >> FSet.removeLock(FactMan, Cp); should happen at the end of the loop, if >> FullyRemove is true. Once that's moved out of the loop, then you can >> simplify some of the if-logic inside the loop as well. Ideally there >> shouldn't be a crash, though; you should get a double-release warning, so >> you may have uncovered a latent bug somewhere else. > > Ah, good catch. Fixed. Thanks! > > One thing that made me slightly unhappy about my previous version of > the patch is that it actually causes a strong increase in code > duplication and actually makes the intersectAndWarn() function pretty > hard to understand. In an attempt to solve this, I have done the > following (also attached): > > http://80386.nl/pub/20140902-clang-scoped_lockable.txt > > Essentially, it does the following: > > - It subclasses the FactEntry into LockableFactEntry and > ScopedLockableFactEntry. For ScopedLockableFactEntry it forces > LK_Exclusive and !Asserted. The Managed field can be moved into > LockableFactEntry. > > - By making FactEntry abstract, we need to patch up certain pieces of > code to use std::unique_ptr, llvm:make_unique, etc. > > - To FactEntry, it adds handleRemovalFromIntersection(). This function > implements a single-directed removal as done by intersectAndWarn(). > The structure of intersectAndWarn() now becomes obvious, as we can use > this operation in both directions now. While there, it fixes a LEK1 vs > LEK2 mixup. > > - Similar to handleRemovalFromIntersection(), it adds handleUnlock(), > which is called by removeLock(). > > Aaron: regarding style: you mentioned clang-format. Do we like > clang-format enough nowadays that I can just reformat the entire file? > If so, shall I perform a separate commit to reformat ThreadSafety.cpp > and rebase the change on top of that?
I would not reformat the entire file at this point (that tends to be pretty noisy for little gain). Instead, you can run clang-format over the patch file itself. There's a pretty simple script for doing this described in the documentation: http://clang.llvm.org/docs/ClangFormat.html Thanks! ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
