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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
