[PATCH] [patch] Adding Consumed Analysis to Clang

[Delesley Hutchins]

  Overall, looks good.  I'm letting David comment on style issues, and I agree with his other concerns.  My comments are specific to the tracking algorithm.


================
Comment at: lib/Analysis/Consumed.cpp:283
@@ +282,3 @@
+namespace {
+class TestedVarsVisitor : public RecursiveASTVisitor<TestedVarsVisitor> {
+  
----------------
I don't think this should be a recursive visitor, or possibly even a visitor at all.  You are only looking for very specific patterns in the AST, rather than visiting every sub-expression.  

================
Comment at: lib/Analysis/Consumed.cpp:491
@@ +490,3 @@
+//       for-loops. (Deferred)
+void ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock,
+                                  const IfStmt *Terminator) {
----------------
// TODO: Handle variable definitions, e.g. bool valid = x.isValid();  if (valid) ...; ...;


================
Comment at: lib/Analysis/Consumed.cpp:412
@@ +411,3 @@
+    
+    if (LocalState == None) {
+      Map.insert(OtherPair);
----------------
David Blaikie wrote:
> Unnecessary { on one expression block
Note that when merging two maps, if a variable is defined in one map, but not the other, then the variable is no longer in scope.  In that case the variable should be removed from the map, rather than added.  

================
Comment at: lib/Analysis/Consumed.cpp:581
@@ +580,3 @@
+      
+      BlockInfo.addInfo(  *SI, CurrStates);
+      BlockInfo.addInfo(*++SI, StatesCopy);
----------------
You need to distinguish here between a successor to a block that you've already processed (which is jump back to the beginning of a loop) and a successor that you haven't yet processed.  In the case of backward jump, you shouldn't do an ordinary merge, you should just check that the assumptions at the end of the loop match those on entry.  

================
Comment at: lib/Analysis/Consumed.cpp:576
@@ +575,3 @@
+      // Handle while- and for-loops.
+      CurrStates->makeUnknown();
+      ConsumedStateMap *StatesCopy = new ConsumedStateMap(*CurrStates);
----------------
Be careful when reusing CurrStates in a new block.  You need to remove the pointer from BlockInfo, otherwise a subsequent back-jump to the current block will end up modifying the variable map for some successor to the current block.  

================
Comment at: lib/Analysis/Consumed.cpp:574
@@ +573,3 @@
+    
+    } else if (CurrBlock->succ_size() == 2) {
+      // Handle while- and for-loops.
----------------
David Blaikie wrote:
> Should this case just be moved to the end of this if/else if chain and just be an unconditional 'else'?
> 
> (& what happens if there are more than 2 successors? (switch block?))
This case should be a general succ_size() > 1 case. 

================
Comment at: lib/Analysis/Consumed.cpp:595
@@ +594,3 @@
+  // Delete the last existing state map.
+  delete CurrStates;
+  
----------------
David Blaikie wrote:
> An owning smart pointer would be nice - perhaps CurrStates should be an OwningPtr<T>?
> 
> Will this lead to a double-delete when you cleanup the contents of BlockInfo (hmm, don't seem to be cleaning those up at all)? I'd like a bit more clear ownership (perhaps keeping the ConsumedStateMaps directly in the BlockInfo, rather than pointers)
As David mentions, you need to write a destructor for BlockInfo; this is a memory leak.


http://llvm-reviews.chandlerc.com/D1233