r209774 - Thread Safety Analysis: update TIL traversal mechanism to allow arbitrary

[Delesley Hutchins]

No prob.  Thanks for the review!  You caught some important bugs.  :-)



On Fri, May 30, 2014 at 10:29 AM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, May 30, 2014 at 1:16 PM, Delesley Hutchins <delesley at google.com> wrote:
>>> The wrapper class kind of weirds me out, truth be told. I get why it's
>>> there, but it seems strange to have this generic wrapper class with a
>>> consistent interface in terms of the methods you call on it, but have
>>> public data members exposed which aren't consistent and can't be used
>>> generically. If we need generic, then we should go generic. If we
>>> don't need generic, CopyReducerBase::Container could be replaced by
>>> SimpleArray<T>.
>>
>> The traverse() methods rely on a generic interface to something that
>> looks like a "container".  In the case of a copyRewrite, it's a real
>> container that maps to a SimpleArray.  In the case of a visitor, it's
>> a fake container, because we don't want to allocate and then destroy
>> lots of unused arrays.
>>
>> The reduceX() methods are responsible for reading from the "container"
>> and then doing something useful with the result.   They are
>> specialized to a particular type of "container."   A visitor gets a
>> fake container and does nothing, while a copyReducer gets a
>> SimpleArray and uses it to allocate new SExprs.  However, the
>> reduceX() methods are defined in some arbitrary derived class, which
>> cannot be friended at the point where Container is declared.
>>
>> The purpose of "container" is merely to act as intermediary between
>> traverse() and reduceX().  IMHO, there's no real reason to worry about
>> privacy here, because the only thing that "container" does is to take
>> the type that is expected by reduceX(), and wrap it in the interface
>> that is used by traverse().
>
> Privacy isn't my concern so much as design. Your explanations make
> sense to me, but it's difficult to envision what's going on when you
> actually look at the code (having not seen this explanation before).
> You see something like copy's container class and go "why does this
> exist when it could just be a SimpleArray?", which comes up when you
> want to extend the existing functionality. When you see visit's
> container, you go "oh, I get it, this is a generic interface!" but
> then you start to notice that there are public members of the
> containers which are totally different.
>
> That being said, I don't think any changes are required at this time.
> Your design explanation makes sense, and I doubt this is an area of
> code where people will be making frequent extensions.
>
> Thanks!
>
> ~Aaron



-- 
DeLesley Hutchins | Software Engineer | delesley at google.com | 505-206-0315