[PATCH] Mostly correct conditional handling for Consumed analysis

Aaron Ballman aaron.ballman at gmail.com
Wed Aug 28 12:03:16 PDT 2013


On Wed, Aug 28, 2013 at 2:43 PM, Delesley Hutchins <delesley at google.com> 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



More information about the cfe-commits mailing list