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

Delesley Hutchins delesley at google.com
Thu May 29 14:33:09 PDT 2014


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.

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

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

  -DeLesley


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



More information about the cfe-commits mailing list