Aaron is right, and this is a blocker for submission of the patch. (Thanks for the catch, Aaron!) The original code didn't have leaks, but the short-circuit returns are introducing them. His suggestion to wrap the pointers in OwningPtr is a good one, and it would prevent future changes from causing this problem. Please update addInfo, too, so that it accepts an OwningPtr reference, and take()s ownership. That would eliminate the need to document the memory management in comments.
-DeLesley On Wed, Aug 28, 2013 at 12:03 PM, Aaron Ballman <[email protected]> wrote: > On Wed, Aug 28, 2013 at 2:43 PM, Delesley Hutchins <[email protected]> > wrote: >>>> > - CS_None, >>>> > + CS_None = 0, >>>> >>>> Any particular reason for this change? >>> >>> So that CS_None evaluates to false if a ConsumedState value is used in a >>> conditional. >>> >>> It would still evaluate to false; it's value when unspecified will be zero. >> >> I don't like relying on auto-conversion from an enum to a bool; for >> readability, I would prefer values of type ConsumedState to be tested >> against a particular enum value. (Otherwise, I have to go look at the >> enum definition to see which value is false.) > > I'm in agreement with that. > >> I've gone over the memory management strategy with Chris, and I >> believe it to be sound. > > Sound is good, but I'm still confused at the reticence over using > managed pointers like OwningPtr. Then you don't need to document the > strategy because the code documents it for you, and it's considerably > safer. > >> >> LGTM, on condition that the enum tests are made explicit, and the >> memory management strategy is suitably documented in a future patch. > > There are still leaks to be solved, and one tiny nit about spacing. > Otherwise, the patch LGTM. > > @@ -694,42 +917,91 @@ > return false; > } > > -// TODO: Handle other forms of branching with precision, including while- and > -// for-loops. (Deferred) > -void ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock, > - const IfStmt *Terminator) { > +bool ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock, > + const ConsumedStmtVisitor &Visitor) { > > - TestedVarsVisitor Visitor(CurrStates); > - Visitor.TraverseStmt(const_cast<Expr*>(Terminator->getCond())); > + ConsumedStateMap *FalseStates = new ConsumedStateMap(*CurrStates); > + PropagationInfo PInfo; > > - bool HasElse = Terminator->getElse() != NULL; > - > - ConsumedStateMap *ElseOrMergeStates = new ConsumedStateMap(*CurrStates); > - > - if (Visitor.IsUsefulConditional) { > - ConsumedState VarState = CurrStates->getState(Visitor.Test.Var); > + if (const IfStmt *IfNode = > + dyn_cast_or_null<IfStmt>(CurrBlock->getTerminator().getStmt())) { > > - if (VarState != CS_Unknown) { > - // FIXME: Make this not warn if the test is from a macro expansion. > - // (Deferred) > - > WarningsHandler.warnUnnecessaryTest(Visitor.Test.Var->getNameAsString(), > - stateToString(VarState), Visitor.Test.Loc); > - } > + bool HasElse = IfNode->getElse() != NULL; > + const Stmt *Cond = IfNode->getCond(); > > - if (Visitor.Test.UnconsumedInTrueBranch) { > - CurrStates->setState(Visitor.Test.Var, CS_Unconsumed); > - if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var, > CS_Consumed); > + PInfo = Visitor.getInfo(Cond); > + if (!PInfo.isValid() && isa<BinaryOperator>(Cond)) > + PInfo = Visitor.getInfo(cast<BinaryOperator>(Cond)->getRHS()); > + > + if (PInfo.isTest()) { > + CurrStates->setSource(Cond); > + FalseStates->setSource(Cond); > + splitVarStateForIf(IfNode, PInfo.getTest(), CurrStates, > + HasElse ? FalseStates : NULL); > + > + } else if (PInfo.isBinTest()) { > + CurrStates->setSource(PInfo.testSourceNode()); > + FalseStates->setSource(PInfo.testSourceNode()); > + splitVarStateForIfBinOp(PInfo, CurrStates, HasElse ? FalseStates > :NULL); > > Missing a space after the colon. > > } else { > - CurrStates->setState(Visitor.Test.Var, CS_Consumed); > - if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var, > CS_Unconsumed); > + return false; > > This is leaking FalseStates > > } > - } > > + } else if (const BinaryOperator *BinOp = > + dyn_cast_or_null<BinaryOperator>(CurrBlock->getTerminator().getStmt())) { > + > + PInfo = Visitor.getInfo(BinOp->getLHS()); > + if (!PInfo.isTest()) { > + if ((BinOp = dyn_cast_or_null<BinaryOperator>(BinOp->getLHS()))) { > + PInfo = Visitor.getInfo(BinOp->getRHS()); > + > + if (!PInfo.isTest()) > + return false; > + > + } else { > + return false; > > Both of these returns are leaking FalseStates. > > + } > + } > + > + CurrStates->setSource(BinOp); > + FalseStates->setSource(BinOp); > + > + const VarTestResult &Test = PInfo.getTest(); > + ConsumedState VarState = CurrStates->getState(Test.Var); > + > + if (BinOp->getOpcode() == BO_LAnd) { > + if (VarState == CS_Unknown) > + CurrStates->setState(Test.Var, Test.TestsFor); > + else if (VarState == invertConsumedUnconsumed(Test.TestsFor)) > + CurrStates->markUnreachable(); > + > + } else if (BinOp->getOpcode() == BO_LOr) { > + if (VarState == CS_Unknown) > + FalseStates->setState(Test.Var, > + invertConsumedUnconsumed(Test.TestsFor)); > + else if (VarState == Test.TestsFor) > + FalseStates->markUnreachable(); > + } > + > + } else { > + return false; > + } > > Leaking FalseStates here as well. > > + > CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin(); > > - if (*SI) BlockInfo.addInfo(*SI, CurrStates); > - if (*++SI) BlockInfo.addInfo(*SI, ElseOrMergeStates); > + if (*SI) > + BlockInfo.addInfo(*SI, CurrStates); > + else > + delete CurrStates; > + > + if (*++SI) > + BlockInfo.addInfo(*SI, FalseStates); > + else > + delete FalseStates; > + > + CurrStates = NULL; > + return true; > } > > ~Aaron -- DeLesley Hutchins | Software Engineer | [email protected] | 505-206-0315 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
