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

[Delesley Hutchins]

Thanks for the long and detailed review!  Responses are below, fixes
are in r205809.

> It seems like a whole lot of these could be forward declares.

This is only included in one spot, so it doesn't matter all that much,
but I've tried to trim down the list.  :-)

>> +// Simple Visitor class for traversing a clang CFG.
>> +class CFGVisitor {
>> ...
>> +};
>
> These all look very much like they should be virtual methods.

No.  The traversal routine is templatized to avoid the overhead of
virtual calls.

> nullptr instead of 0?

Fixed, here and elsewhere.

>> +  ~CFGWalker() { }
>
> Why is this required?

It's not.  Deleted.

>> +  // Initialize the CFGWalker.  This setup only needs to be done once, even
>> +  // if there are multiple passes over the CFG.
>> +  bool init(AnalysisDeclContext &AC) {
>
> Why is this not a constructor when it only needs to be done once?

I want to decouple construction and initialization here.

>> +      for (CFGBlock::const_iterator BI = CurrBlock->begin(),
>> +                                    BE = CurrBlock->end();
>> +           BI != BE; ++BI) {
>
> Range-based for loop?

Done.

> Getting rid of these const_casts would be wonderful, if possible.

I agree, but it's not possible in this case.

> Why are these all public?

Because of the way things are hooked together right now.  Added TODO;
will fix later.

> It seems like a lot of these methods shouldn't be public.

Switched to protected.

> Why protected?

So it's possible to create derived builders.

>> +// Dump an SCFG to llvm::errs().
>> +void printSCFG(CFGWalker &walker);
>
> This seems like it'd be more reasonable as a dump() method on
> CFGWalker than a stand-alone function.

I disagree.  CFGWalker is a tool to walk a clang::CFG, and is designed
for multiple passes with different visitors.  printSCFG constructs and
prints a til::SCFG.  It is mainly for testing purposes, and should not
be part of the public interface of CFGWalker.

> Everything in this file is in the til namespace, yet it has an odd
> mixture of naming conventions used. Eg) til::TILPrettyPrinter,

Good point.  I fixed most of these.  I kept the opcode ones, because I
like green bike sheds.  :-)

> Can an existing container class suffice? I'm not keen on adding
> another container data type, much less exposing it like this.

I will switch to a different container in a future patch.

> You're kind of missing a virtual destructor here. Future inherits from
> SExpr and exposes virtual functions, so polymorphic deletion of a
> Future through a static type of SExpr would be problematic.

No, I very much do not want a virtual method table, because SExprs are
supposed to be very small, bump-allocated classes, and I have gone to
great lengths to avoid the extra overhead.  Moreover, because they use
bump-allocation, SExpr destructors are never called.  Future is the
only class which has a virtual method, and where I'm willing to pay
the memory penalty for having it.

> This isn't a typical pattern used in the code base, and it makes it
> harder to write const-correct code.

Already fixed in my last patch.

> expr() instead of clangExpr()? A bit of a bikeshed, but the name is
> just kind of odd to me.

No.  expr() in other classes returns an SExpr, whereas this method
links back to clang::Expr.

> decl() instead of clangDecl() for the same reasoning as above?

No.  Same reason as before.  The TIL is a separate language, so any
links back to clang are spelled out for clarity.

> Why _x?

The default name for an unnamed variable is always 'x', but there
might be real variables with that name, so I add a leading underscore.
 :-)

> Should there be an assert that NumUses >= 0 when detaching (to catch
> mismatches)?

Good idea.  Added.

> Seems like NumUses should be unsigned.

Done.  Now that there's an assert, I'm more comfortable with that decision.  :-)

> I would really rather not see a constructor which steals memory from
> something else -- could it be a factory function instead, so it's at
> least a little more readily apparent to callers?

I've changed these to use move semantics for clarity.  (As you have
noticed, my first patch had very little C++11.)  A factory method
would break the pattern used by all of the other constructors.

> This would be more clear with reverse iterators (until we get a
> reverse range adapter).

Added TODO.

>> +  void setArena(MemRegionRef A) { Arena = A; }
>
> This seems like it could be used as a constructor parameter instead.

Can't, without making it a constructor parameter of Traversal, which
is intended for use with classes that don't involve MemRegions.

>> +// Basic class for comparison operations over expressions.
>> +template <typename Self>
>> +class TILComparator {
>> +public:
>> +  Self *self() { return reinterpret_cast<Self *>(this); }
>
> Does this need to be public?

Changed to protected.

> Why not private?

For future extension.  There is more than one way to pretty print an expression.

> Why not an enumeration instead?

Mainly because I'm doing arithmetic with them.

> These two lines can be removed entirely instead of left commented out.

Except that I constantly uncomment them for testing.  :-)

> Seems like CallExpr could use an arguments() range function that could
> be used here.  ;-)

True.  :-)  As could several CFG routines.

>> +// Build a complete SCFG from a clang CFG.
>> +class SCFGBuilder : public CFGVisitor {
>> +public:
>> +  // return true if E should be included in the SCFG
>> +  bool includeExpr(til::SExpr* E) {
>
> This should be private. That goes for other members of this class as well.

Done.

> These are being leaked. unique_ptr perhaps?

Nice catch!  Thanks!  Fixed.

> All of these blocks wind up being leaked.

Thanks again.  Fixed.


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