<div dir="ltr"><div><br></div><div>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.</div>
<div><br></div><div>Another minor nit is that underlying() returns a pointer.  If it can never be null, then it should return a reference.<br></div><div><br></div><div>Other than that, this patch looks good to me.  Thanks!  I'll defer to Aaron regarding all style issues.  However, Ed is right that it doesn't matter whether the guard object is exclusive or shared.  I also don't think there's a problem with the corner case.  If there are no underlying locks, then the correct behavior is to lock the guard object on construction, and unlock it on destruction, which is exactly what this patch does, and what the original corner case was designed to handle.</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 7:20 AM, 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">Hi Aaron,<br>
<br>
First of all, thanks for your comments! I'll address the formatting<br>
issues and the crash bug ASAP. Weird that it didn't do this here.<br>
<div class=""><br>
On 2 September 2014 14:56, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> This adds all locks to the analyzer as exclusive locks, but the<br>
> previous code would handled shared locks separately. Is that<br>
> intentional?<br>
<br>
</div>So the intent behind this change is that regardless of the number of<br>
locks that are picked up by the guard object, it is tracked as always<br>
single acquisition of the lock guard object itself. The lock on the<br>
guard object will reference all of the actual underlying locks that<br>
are performed, that are either locked exclusively or shared.<br>
<br>
In short, to my knowledge it is irrelevant whether the lock on the<br>
guard object is shared or exclusive. The guard object gets locked upon<br>
construction and unlocked upon destruction. This is why I just picked<br>
exclusive.<br>
<br>
Now that you mention it, this change will indeed break if there are no<br>
underlying locks. The set of locks will be empty and our check against<br>
empty() to determine whether we are a scoped guard object or not will<br>
be incorrect.<br>
<br>
You know what? Let me take a look at the existing FactEntry class and<br>
see if we can introduce some polymorphism here. Ideally, we should<br>
treat "lockable" and "scoped_lockable" objects completely separately.<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>