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

[Aaron Ballman]

On Thu, May 29, 2014 at 5:33 PM, Delesley Hutchins <delesley at google.com> wrote:
> Thanks for the review!  Fixed in r209847.
>
>> Why the access change? As best I can tell, it's not required.
>
> It doesn't need to be.  Done.
>
>> Instead of all of these static_casts, would it make more sense to have
>> a templated function on Literal? Eg)
>
> done.
>
>> auto I = std::find(Predecessors.begin(), Predecessors.end(), BB);
>> return std::distance(Predecessors.begin(), I);
>>
>> Also, BB and the function itself can be const.
>
> done.
>
>> These should all be const functions.
>
> Not really.  The trivial definitions here are const, but they may be
> non-const in other implementations.

Fair enough.

>
>> Why is Elems now public?
>
> The inheritance hierarchy has changed, so the friend statement no longer works.
> This is a trivial wrapper class; privacy is not really important here.
>
>> Why is this member public?
>
> Same reason as Elems earlier.

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>.

>
>> This doesn't seem quite right. If Capacity is 0, what guarantees are
>> there that InitialCapacity > N? Also, what guarantees that Capacity *
>> 2 is large enough?
>
> An excellent catch!  Thanks!  Done.  Test coverage was poor: I always
> call it with N==1.  :-)
>
>> This is potentially UB; BB can be nullptr from the preceding statement.
>
> Another excellent catch.  Thanks!  Done.
>
>> Same sort of UB here as above.
>
> Why?  BB1 and BB2 are checked for nullptr.

Why? Because my eyes don't work properly, that's why!  ;-) You're
absolutely correct, no UB there.

Thanks!

~Aaron