<div dir="ltr"><br><div>I think this is a generally useful refactoring, especially since it lays the groundwork for adding different kinds of FactEntries; Thanks!  Two comments:</div><div><br></div><div>(1) FactEntry needs a virtual destructor to avoid memory leaks.</div>
<div>(2) I would like the different FactEntry classes to be declared together (at the top of the file), rather than split as they are currently.  </div><div><br></div><div>Other than that, looks good.  </div><div><br></div>
<div>  -DeLesley</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Sep 2, 2014 at 12:12 PM, Ed Schouten <span dir="ltr"><<a href="mailto:ed@80386.nl" target="_blank">ed@80386.nl</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On 2 September 2014 20:56, Delesley Hutchins <<a href="mailto:delesley@google.com">delesley@google.com</a>> wrote:<br>

> The crash is probably being caused by the fact that in removeLock(), you are<br>
> trying to remove the guard object multiple times.  The call to<br>
> FSet.removeLock(FactMan, Cp);  should happen at the end of the loop, if<br>
> FullyRemove is true.  Once that's moved out of the loop, then you can<br>
> simplify some of the if-logic inside the loop as well.  Ideally there<br>
> shouldn't be a crash, though; you should get a double-release warning, so<br>
> you may have uncovered a latent bug somewhere else.<br>
<br>
</div>Ah, good catch. Fixed. Thanks!<br>
<br>
One thing that made me slightly unhappy about my previous version of<br>
the patch is that it actually causes a strong increase in code<br>
duplication and actually makes the intersectAndWarn() function pretty<br>
hard to understand. In an attempt to solve this, I have done the<br>
following (also attached):<br>
<br>
<a href="http://80386.nl/pub/20140902-clang-scoped_lockable.txt" target="_blank">http://80386.nl/pub/20140902-clang-scoped_lockable.txt</a><br>
<br>
Essentially, it does the following:<br>
<br>
- It subclasses the FactEntry into LockableFactEntry and<br>
ScopedLockableFactEntry. For ScopedLockableFactEntry it forces<br>
LK_Exclusive and !Asserted. The Managed field can be moved into<br>
LockableFactEntry.<br>
<br>
- By making FactEntry abstract, we need to patch up certain pieces of<br>
code to use std::unique_ptr, llvm:make_unique, etc.<br>
<br>
- To FactEntry, it adds handleRemovalFromIntersection(). This function<br>
implements a single-directed removal as done by intersectAndWarn().<br>
The structure of intersectAndWarn() now becomes obvious, as we can use<br>
this operation in both directions now. While there, it fixes a LEK1 vs<br>
LEK2 mixup.<br>
<br>
- Similar to handleRemovalFromIntersection(), it adds handleUnlock(),<br>
which is called by removeLock().<br>
<br>
Aaron: regarding style: you mentioned clang-format. Do we like<br>
clang-format enough nowadays that I can just reformat the entire file?<br>
If so, shall I perform a separate commit to reformat ThreadSafety.cpp<br>
and rebase the change on top of that?<br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Ed Schouten <<a href="mailto:ed@80386.nl">ed@80386.nl</a>><br>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br>DeLesley Hutchins | Software Engineer | <a href="mailto:delesley@google.com" target="_blank">delesley@google.com</a> | 505-206-0315<br>
</div>