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

[Aaron Ballman]

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