r205728 - Thread Safety Analysis: update to internal SExpr handling.

[Aaron Ballman]

On Wed, Apr 9, 2014 at 12:45 PM, Delesley Hutchins <delesley at google.com> wrote:
> Patch looks good overall, except for the constness of
> handleDestructorCall, and SExprRef below.  You are free to submit
> (with those fixes); I won't update ThreadSafetyTIL until the patch
> goes in.

I've committed with your suggestions in r205915

>> That means this entire class is mostly for documentation. In the
>> attached patch, I simply commented it out and explained it a bit
>> further.
>
> The advantage of keeping it a class is that you can inherit from it
> and only "override" one or two methods, rather than having to
> implement the whole interface.  The disadvantage is that if you get
> the type signature wrong, the overriding doesn't happen and there's no
> warning.   I could go either way on this.  You can keep it commented
> if you prefer.

I left it commented out, but definitely see your point. If it becomes
more useful in the future, we can turn it back into a base class
easily enough.

>
>> My attach patch managed it (but perhaps that's just because I was
>> testing with MSVC).
>
> This works because I goofed.  :-)  handleDestructorCall will
> eventually need to take non-const arguments, because I'd like this
> traversal to eventually replace the main runAnalysis() routine in
> ThreadSafety.cpp.  (The casts are copied from there, where they are
> needed.)

Ahhh, okay, that makes more sense then!

>
>> That's kind of what I figured, but what's strange to me is that SExpr
>> (and subclasses) seem to always be created in the Arena -- so the
>> destructors aren't being called anyway (I didn't see any explicit
>> destructor calls happening). That's why my proposed patch tries to
>> make this more clear -- you must create them in an arena, and you
>> cannot delete them.
>
> This is an excellent fix.  Thanks!
>
>> Shouldn't that be defined as a swap (since right now, it's just
>> copying the pointer)?
>>
>> SExprRef(SExprRef &&R) { std::swap(Ptr, R.Ptr); }
>
> No.  My bad.  This should be changed to:
>
>   SExprRef(SExprRef &&R) : Ptr(R.Ptr) { R.Ptr = nullptr; }

Fixed.

>
> Also, please put back the nullptr constructor.  Null SExprRefs are
> allowed, and the null constructor cuts out some overhead.

Also fixed.

> Perhaps the copy constructor should be explicitly deleted?

I didn't implement this, but wouldn't be opposed to doing it. It's
implicitly deleted already (because of the move constructor), so any
attempts to use it would be diagnosed.

> Thanks again for the review; lots of good comments.

My pleasure -- thank you for all the awesome work on this, it looks
really great!

~Aaron