[PATCH] Branch handling cleanup in Consumed analysis

Aaron Ballman aaron.ballman at gmail.com
Thu Aug 22 20:14:50 PDT 2013


In addition to David's concerns about the bit twiddling, some comments
below.  Some minor nitpicking included.

> Index: include/clang/Analysis/Analyses/Consumed.h
> ===================================================================
> --- include/clang/Analysis/Analyses/Consumed.h
> +++ include/clang/Analysis/Analyses/Consumed.h
> @@ -25,6 +25,8 @@
>  namespace clang {
>  namespace consumed {
>
> +  class ConsumedStmtVisitor;
> +
>    typedef SmallVector<PartialDiagnosticAt, 1> OptionalNotes;
>    typedef std::pair<PartialDiagnosticAt, OptionalNotes> DelayedDiag;
>    typedef std::list<DelayedDiag> DiagList;
> @@ -90,11 +92,11 @@
>
>    enum ConsumedState {
>      // No state information for the given variable.
> -    CS_None,
> +    CS_None = 0,
>
> -    CS_Unknown,
> -    CS_Unconsumed,
> -    CS_Consumed
> +    CS_Unknown    = 1,
> +    CS_Unconsumed = 2,
> +    CS_Consumed   = 3
>    };
>
>    class ConsumedStateMap {
> @@ -144,18 +146,6 @@
>
>      void markVisited(const CFGBlock *Block);
>    };
> -
> -  struct VarTestResult {
> -    const VarDecl *Var;
> -    SourceLocation Loc;
> -    bool UnconsumedInTrueBranch;
> -
> -    VarTestResult() : Var(NULL), Loc(), UnconsumedInTrueBranch(true) {}
> -
> -    VarTestResult(const VarDecl *Var, SourceLocation Loc,
> -                  bool UnconsumedInTrueBranch)
> -      : Var(Var), Loc(Loc), UnconsumedInTrueBranch(UnconsumedInTrueBranch) {}
> -  };
>
>    /// A class that handles the analysis of uniqueness violations.
>    class ConsumedAnalyzer {
> @@ -169,7 +159,8 @@
>      CacheMapType ConsumableTypeCache;
>
>      bool hasConsumableAttributes(const CXXRecordDecl *RD);
> -    void splitState(const CFGBlock *CurrBlock, const IfStmt *Terminator);
> +    void splitState(const ConsumedStmtVisitor &Visitor,
> +                    const CFGBlock *CurrBlock, const IfStmt *Terminator);
>
>    public:
>
> Index: lib/Analysis/Consumed.cpp
> ===================================================================
> --- lib/Analysis/Consumed.cpp
> +++ lib/Analysis/Consumed.cpp
> @@ -32,6 +32,8 @@
>
>  // TODO: Mark variables as Unknown going into while- or for-loops only if they
>  //       are referenced inside that block. (Deferred)
> +// TODO: Handle variable definitions, e.g. bool valid = x.isValid();
> +//       if (valid) ...; (Deferred)
>  // TODO: Add a method(s) to identify which method calls perform what state
>  //       transitions. (Deferred)
>  // TODO: Take notes on state transitions to provide better warning messages.
> @@ -41,6 +43,8 @@
>  // TODO: Test IsFalseVisitor with values in the unknown state. (Deferred)
>  // TODO: Look into combining IsFalseVisitor and TestedVarsVisitor. (Deferred)
>
> +#define INVERT_CONSUMED_UNCONSUMED(State) static_cast<ConsumedState>(State ^ 1)
> +
>  using namespace clang;
>  using namespace consumed;
>
> @@ -69,40 +73,79 @@
>  }
>
>  namespace {
> +struct VarTestResult {
> +  const VarDecl *Var;
> +  SourceLocation Loc;
> +  ConsumedState TrueBranchState;
> +
> +  VarTestResult() : Var(NULL), Loc(), TrueBranchState(CS_None) {}
> +
> +  VarTestResult(const VarDecl *Var, SourceLocation Loc,
> +                ConsumedState TrueBranchState)
> +    : Var(Var), Loc(Loc), TrueBranchState(TrueBranchState) {}
> +};
> +} // end anonymous::VarTestResult
> +
> +namespace clang {
> +namespace consumed {
>  class ConsumedStmtVisitor : public ConstStmtVisitor<ConsumedStmtVisitor> {
>
> -  union PropagationUnion {
> -    ConsumedState State;
> -    const VarDecl *Var;
> +  enum ConstantInfo {
> +    CV_None,
> +    CV_True,
> +    CV_False
>    };
>
> -  class PropagationInfo {
> -    PropagationUnion StateOrVar;
> +  struct RangeInfo {
> +    unsigned Begin, End;

It's generally considered better to use a specific type instead of
simply signed/unsigned.  You'll find code which doesn't, but I think
the rule of thumb is that new code shouldn't do this.  That should
propagate to other places in the patch (pun only mildly intended).

> +    ConstantInfo ConstantValue;
> +  };
>
> -  public:
> -    bool IsVar;
> +  class PropagationInfo {
> +    union PropagationUnion {
> +      ConsumedState State;
> +      RangeInfo Range;
> +      const VarDecl *Var;
> +    } InfoUnion;
>
> -    PropagationInfo() : IsVar(false) {
> -      StateOrVar.State = consumed::CS_None;
> +  public:
> +    enum {
> +      IT_None,
> +      IT_State,
> +      IT_Range,
> +      IT_Var
> +    } InfoType;
> +
> +    PropagationInfo() : InfoType(IT_None) {}
> +
> +    PropagationInfo(unsigned RBegin, unsigned REnd) : InfoType(IT_Range) {
> +      InfoUnion.Range.Begin = RBegin;
> +      InfoUnion.Range.End   = REnd;
> +      InfoUnion.Range.ConstantValue = CV_None;
>      }
>
> -    PropagationInfo(ConsumedState State) : IsVar(false) {
> -      StateOrVar.State = State;
> +    PropagationInfo(ConsumedState State) : InfoType(IT_State) {
> +      InfoUnion.State = State;
>      }
>
> -    PropagationInfo(const VarDecl *Var) : IsVar(true) {
> -      StateOrVar.Var = Var;
> +    PropagationInfo(const VarDecl *Var) : InfoType(IT_Var) {
> +      InfoUnion.Var = Var;
>      }
>
> -    ConsumedState getState() const { return StateOrVar.State; }
> +    const ConsumedState & getState() const { return InfoUnion.State; }
> +    const RangeInfo     & getRange() const { return InfoUnion.Range; }
> +    const VarDecl       * getVar()   const { return InfoUnion.Var; }

The spacing here is kind of distracting, especially for the VarDecl.

>
> -    const VarDecl * getVar() const { return IsVar ? StateOrVar.Var : NULL; }
> +    bool isState() const { return InfoType == IT_State; }
> +    bool isRange() const { return InfoType == IT_Range; }
> +    bool isVar()   const { return InfoType == IT_Var; }
>    };
>
>    typedef llvm::DenseMap<const Stmt *, PropagationInfo> MapType;
>    typedef std::pair<const Stmt *, PropagationInfo> PairType;
>    typedef MapType::iterator InfoEntry;
> -
> +  typedef MapType::const_iterator ConstInfoEntry;
> +
>    AnalysisDeclContext &AC;
>    ConsumedAnalyzer &Analyzer;
>    ConsumedStateMap *StateMap;
> @@ -115,6 +158,8 @@
>    bool isLikeMoveAssignment(const CXXMethodDecl *MethodDecl);
>
>  public:
> +
> +  llvm::SmallVector<VarTestResult, 5> TestedVars;
>
>    void Visit(const Stmt *StmtNode);
>
> @@ -134,22 +179,34 @@
>    ConsumedStmtVisitor(AnalysisDeclContext &AC, ConsumedAnalyzer &Analyzer,
>                        ConsumedStateMap *StateMap)
>        : AC(AC), Analyzer(Analyzer), StateMap(StateMap) {}
> -
> +
> +  std::pair<unsigned, unsigned> getTestRange(const Stmt *StmtNode) const {
> +    ConstInfoEntry Entry = PropagationMap.find(StmtNode);
> +
> +    if (Entry != PropagationMap.end() && Entry->second.isRange()) {
> +      return std::make_pair(Entry->second.getRange().Begin,
> +                            Entry->second.getRange().End);
> +    } else {
> +      return std::make_pair(0, 0);
> +    }
> +  }
> +
>    void reset() {
>      PropagationMap.clear();
> +    TestedVars.clear();
>    }
>  };
>
>  // TODO: When we support CallableWhenConsumed this will have to check for
>  //       the different attributes and change the behavior bellow. (Deferred)
> -void ConsumedStmtVisitor::checkCallability(const PropagationInfo &PState,
> +void ConsumedStmtVisitor::checkCallability(const PropagationInfo &PInfo,
>                                             const FunctionDecl *FunDecl,
>                                             const CallExpr *Call) {
>
>    if (!FunDecl->hasAttr<CallableWhenUnconsumedAttr>()) return;
>
> -  if (PState.IsVar) {
> -    const VarDecl *Var = PState.getVar();
> +  if (PInfo.isVar()) {
> +    const VarDecl *Var = PInfo.getVar();
>
>      switch (StateMap->getState(Var)) {
>      case CS_Consumed:
> @@ -170,7 +227,7 @@
>      }
>
>    } else {
> -    switch (PState.getState()) {
> +    switch (PInfo.getState()) {
>      case CS_Consumed:
>        Analyzer.WarningsHandler.warnUseOfTempWhileConsumed(
>          FunDecl->getNameAsString(), Call->getExprLoc());
> @@ -250,24 +307,24 @@
>
>        InfoEntry Entry = PropagationMap.find(Call->getArg(Index));
>
> -      if (Entry == PropagationMap.end() || !Entry->second.IsVar) {
> +      if (Entry == PropagationMap.end() || !Entry->second.isVar()) {
>          continue;
>        }
>
> -      PropagationInfo PState = Entry->second;
> +      PropagationInfo PInfo = Entry->second;
>
>        if (ParamType->isRValueReferenceType() ||
>            (ParamType->isLValueReferenceType() &&
>             !cast<LValueReferenceType>(*ParamType).isSpelledAsLValue())) {
>
> -        StateMap->setState(PState.getVar(), consumed::CS_Consumed);
> +        StateMap->setState(PInfo.getVar(), consumed::CS_Consumed);
>
>        } else if (!(ParamType.isConstQualified() ||
>                     ((ParamType->isReferenceType() ||
>                       ParamType->isPointerType()) &&
>                      ParamType->getPointeeType().isConstQualified()))) {
>
> -        StateMap->setState(PState.getVar(), consumed::CS_Unknown);
> +        StateMap->setState(PInfo.getVar(), consumed::CS_Unknown);
>        }
>      }
>    }
> @@ -295,11 +352,11 @@
>
>      } else if (Constructor->isMoveConstructor()) {
>
> -      PropagationInfo PState =
> +      PropagationInfo PInfo =
>          PropagationMap.find(Call->getArg(0))->second;
>
> -      if (PState.IsVar) {
> -        const VarDecl* Var = PState.getVar();
> +      if (PInfo.isVar()) {
> +        const VarDecl* Var = PInfo.getVar();
>
>          PropagationMap.insert(PairType(Call,
>            PropagationInfo(StateMap->getState(Var))));
> @@ -307,7 +364,7 @@
>          StateMap->setState(Var, consumed::CS_Consumed);
>
>        } else {
> -        PropagationMap.insert(PairType(Call, PState));
> +        PropagationMap.insert(PairType(Call, PInfo));
>        }
>
>      } else if (Constructor->isCopyConstructor()) {
> @@ -331,16 +388,27 @@
>    InfoEntry Entry = PropagationMap.find(Call->getCallee()->IgnoreParens());
>
>    if (Entry != PropagationMap.end()) {
> -    PropagationInfo PState = Entry->second;
> +    PropagationInfo PInfo = Entry->second;
>      const CXXMethodDecl *MethodDecl = Call->getMethodDecl();
>
> -    checkCallability(PState, MethodDecl, Call);
> +    checkCallability(PInfo, MethodDecl, Call);
>
> -    if (PState.IsVar) {
> -      if (MethodDecl->hasAttr<ConsumesAttr>())
> -        StateMap->setState(PState.getVar(), consumed::CS_Consumed);
> -      else if (!MethodDecl->isConst())
> -        StateMap->setState(PState.getVar(), consumed::CS_Unknown);
> +    if (PInfo.isVar()) {
> +      if (isTestingFunction(MethodDecl)) {
> +        unsigned short CurrIndex = TestedVars.size();
> +
> +        PropagationMap.insert(PairType(Call,
> +          PropagationInfo(CurrIndex, CurrIndex + 1)));
> +
> +        TestedVars.push_back(
> +          VarTestResult(PInfo.getVar(), Call->getExprLoc(), CS_Unconsumed));
> +
> +      } else if (MethodDecl->hasAttr<ConsumesAttr>()) {
> +        StateMap->setState(PInfo.getVar(), consumed::CS_Consumed);
> +
> +      } else if (!MethodDecl->isConst()) {
> +        StateMap->setState(PInfo.getVar(), consumed::CS_Unknown);
> +      }
>      }
>    }
>  }
> @@ -359,46 +427,46 @@
>      InfoEntry LEntry = PropagationMap.find(Call->getArg(0));
>      InfoEntry REntry = PropagationMap.find(Call->getArg(1));
>
> -    PropagationInfo LPState, RPState;
> +    PropagationInfo LPInfo, RPInfo;
>
>      if (LEntry != PropagationMap.end() &&
>          REntry != PropagationMap.end()) {
>
> -      LPState = LEntry->second;
> -      RPState = REntry->second;
> +      LPInfo = LEntry->second;
> +      RPInfo = REntry->second;
>
> -      if (LPState.IsVar && RPState.IsVar) {
> -        StateMap->setState(LPState.getVar(),
> -          StateMap->getState(RPState.getVar()));
> +      if (LPInfo.isVar() && RPInfo.isVar()) {
> +        StateMap->setState(LPInfo.getVar(),
> +          StateMap->getState(RPInfo.getVar()));
>
> -        StateMap->setState(RPState.getVar(), consumed::CS_Consumed);
> +        StateMap->setState(RPInfo.getVar(), consumed::CS_Consumed);
>
> -        PropagationMap.insert(PairType(Call, LPState));
> +        PropagationMap.insert(PairType(Call, LPInfo));
>
> -      } else if (LPState.IsVar && !RPState.IsVar) {
> -        StateMap->setState(LPState.getVar(), RPState.getState());
> +      } else if (LPInfo.isVar() && !RPInfo.isVar()) {
> +        StateMap->setState(LPInfo.getVar(), RPInfo.getState());
>
> -        PropagationMap.insert(PairType(Call, LPState));
> +        PropagationMap.insert(PairType(Call, LPInfo));
>
> -      } else if (!LPState.IsVar && RPState.IsVar) {
> +      } else if (!LPInfo.isVar() && RPInfo.isVar()) {
>          PropagationMap.insert(PairType(Call,
> -          PropagationInfo(StateMap->getState(RPState.getVar()))));
> +          PropagationInfo(StateMap->getState(RPInfo.getVar()))));
>
> -        StateMap->setState(RPState.getVar(), consumed::CS_Consumed);
> +        StateMap->setState(RPInfo.getVar(), consumed::CS_Consumed);
>
>        } else {
> -        PropagationMap.insert(PairType(Call, RPState));
> +        PropagationMap.insert(PairType(Call, RPInfo));
>        }
>
>      } else if (LEntry != PropagationMap.end() &&
>                 REntry == PropagationMap.end()) {
>
> -      LPState = LEntry->second;
> +      LPInfo = LEntry->second;
>
> -      if (LPState.IsVar) {
> -        StateMap->setState(LPState.getVar(), consumed::CS_Unknown);
> +      if (LPInfo.isVar()) {
> +        StateMap->setState(LPInfo.getVar(), consumed::CS_Unknown);
>
> -        PropagationMap.insert(PairType(Call, LPState));
> +        PropagationMap.insert(PairType(Call, LPInfo));
>
>        } else {
>          PropagationMap.insert(PairType(Call,
> @@ -408,10 +476,10 @@
>      } else if (LEntry == PropagationMap.end() &&
>                 REntry != PropagationMap.end()) {
>
> -      RPState = REntry->second;
> +      RPInfo = REntry->second;
>
> -      if (RPState.IsVar) {
> -        const VarDecl *Var = RPState.getVar();
> +      if (RPInfo.isVar()) {
> +        const VarDecl *Var = RPInfo.getVar();
>
>          PropagationMap.insert(PairType(Call,
>            PropagationInfo(StateMap->getState(Var))));
> @@ -419,7 +487,7 @@
>          StateMap->setState(Var, consumed::CS_Consumed);
>
>        } else {
> -        PropagationMap.insert(PairType(Call, RPState));
> +        PropagationMap.insert(PairType(Call, RPInfo));
>        }
>      }
>
> @@ -430,20 +498,30 @@
>      InfoEntry Entry = PropagationMap.find(Call->getArg(0));
>
>      if (Entry != PropagationMap.end()) {
> -      PropagationInfo PState = Entry->second;
> +      PropagationInfo PInfo = Entry->second;
>
> -      checkCallability(PState, FunDecl, Call);
> +      checkCallability(PInfo, FunDecl, Call);
>
> -      if (PState.IsVar) {
> -        if (FunDecl->hasAttr<ConsumesAttr>()) {
> +      if (PInfo.isVar()) {
> +        if (isTestingFunction(FunDecl)) {
> +          unsigned short CurrIndex = TestedVars.size();
> +
> +          PropagationMap.insert(PairType(Call,
> +            PropagationInfo(CurrIndex, CurrIndex + 1)));
> +
> +          TestedVars.push_back(
> +            VarTestResult(PInfo.getVar(), Call->getExprLoc(), CS_Unconsumed));
> +
> +        } else if (FunDecl->hasAttr<ConsumesAttr>()) {
>            // Handle consuming operators.
> -          StateMap->setState(PState.getVar(), consumed::CS_Consumed);
> +          StateMap->setState(PInfo.getVar(), consumed::CS_Consumed);
> +
>          } else if (const CXXMethodDecl *MethodDecl =
>            dyn_cast_or_null<CXXMethodDecl>(FunDecl)) {
>
>            // Handle non-constant member operators.
>            if (!MethodDecl->isConst())
> -            StateMap->setState(PState.getVar(), consumed::CS_Unknown);
> +            StateMap->setState(PInfo.getVar(), consumed::CS_Unknown);
>          }
>        }
>      }
> @@ -482,84 +560,42 @@
>  }
>
>  void ConsumedStmtVisitor::VisitUnaryOperator(const UnaryOperator *UOp) {
> -  if (UOp->getOpcode() == UO_AddrOf) {
> -    InfoEntry Entry = PropagationMap.find(UOp->getSubExpr());
> -
> -    if (Entry != PropagationMap.end())
> -      PropagationMap.insert(PairType(UOp, Entry->second));
> -  }
> -}
> -
> -void ConsumedStmtVisitor::VisitVarDecl(const VarDecl *Var) {
> -  if (Analyzer.isConsumableType(Var->getType())) {
> -    PropagationInfo PState =
> -      PropagationMap.find(Var->getInit())->second;
> -
> -    StateMap->setState(Var, PState.IsVar ?
> -      StateMap->getState(PState.getVar()) : PState.getState());
> -  }
> -}
> -} // end anonymous::ConsumedStmtVisitor
> -
> -namespace {
> -
> -// TODO: Handle variable definitions, e.g. bool valid = x.isValid();
> -//       if (valid) ...; (Deferred)
> -class TestedVarsVisitor : public RecursiveASTVisitor<TestedVarsVisitor> {
> -
> -  bool Invert;
> -  SourceLocation CurrTestLoc;
> +  InfoEntry Entry = PropagationMap.find(UOp->getSubExpr());
> +  if (Entry == PropagationMap.end()) return;
>
> -  ConsumedStateMap *StateMap;
> -
> -public:
> -  bool IsUsefulConditional;
> -  VarTestResult Test;
> -
> -  TestedVarsVisitor(ConsumedStateMap *StateMap) : Invert(false),
> -    StateMap(StateMap), IsUsefulConditional(false) {}
> +  switch (UOp->getOpcode()) {
> +  case UO_AddrOf:
> +    PropagationMap.insert(PairType(UOp, Entry->second));
> +    break;
>
> -  bool VisitCallExpr(CallExpr *Call);
> -  bool VisitDeclRefExpr(DeclRefExpr *DeclRef);
> -  bool VisitUnaryOperator(UnaryOperator *UnaryOp);
> -};
> -
> -bool TestedVarsVisitor::VisitCallExpr(CallExpr *Call) {
> -  if (const FunctionDecl *FunDecl =
> -    dyn_cast_or_null<FunctionDecl>(Call->getDirectCallee())) {
> -
> -    if (isTestingFunction(FunDecl)) {
> -      CurrTestLoc = Call->getExprLoc();
> -      IsUsefulConditional = true;
> -      return true;
> +  case UO_LNot:
> +    if (Entry->second.isRange()) {
> +      const RangeInfo &Range = Entry->second.getRange();
> +
> +      for (unsigned index = Range.Begin; index < Range.End; ++index) {
> +        ConsumedState &TestState  = TestedVars[index].TrueBranchState;
> +        TestState = INVERT_CONSUMED_UNCONSUMED(TestState);
> +      }
> +
> +      PropagationMap.insert(PairType(UOp, PropagationInfo(Entry->second)));
>      }
> -
> -    IsUsefulConditional = false;
> -  }
> -
> -  return false;
> -}
> -
> -bool TestedVarsVisitor::VisitDeclRefExpr(DeclRefExpr *DeclRef) {
> -  if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(DeclRef->getDecl()))
> -    if (StateMap->getState(Var) != consumed::CS_None)
> -      Test = VarTestResult(Var, CurrTestLoc, !Invert);
> +    break;
>
> -  return true;
> +  default:
> +    break;
> +  }
>  }
>
> -bool TestedVarsVisitor::VisitUnaryOperator(UnaryOperator *UnaryOp) {
> -  if (UnaryOp->getOpcode() == UO_LNot) {
> -    Invert = true;
> -    TraverseStmt(UnaryOp->getSubExpr());
> +void ConsumedStmtVisitor::VisitVarDecl(const VarDecl *Var) {
> +  if (Analyzer.isConsumableType(Var->getType())) {
> +    PropagationInfo PInfo =
> +      PropagationMap.find(Var->getInit())->second;
>
> -  } else {
> -    IsUsefulConditional = false;
> +    StateMap->setState(Var, PInfo.isVar() ?
> +      StateMap->getState(PInfo.getVar()) : PInfo.getState());
>    }
> -
> -  return false;
>  }
> -} // end anonymouse::TestedVarsVisitor
> +}} // end clang::consumed::ConsumedStmtVisitor

Missing a newline between the curly braces.

>
>  namespace clang {
>  namespace consumed {
> @@ -696,33 +732,37 @@
>
>  // TODO: Handle other forms of branching with precision, including while- and
>  //       for-loops. (Deferred)
> -void ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock,
> +void ConsumedAnalyzer::splitState(const ConsumedStmtVisitor &Visitor,
> +                                  const CFGBlock *CurrBlock,
>                                    const IfStmt *Terminator) {
>
> -  TestedVarsVisitor Visitor(CurrStates);
> -  Visitor.TraverseStmt(const_cast<Expr*>(Terminator->getCond()));
> +  bool HasElse                        = Terminator->getElse() != NULL;

The whitespace doesn't really seem to add anything to the readability.

> +  ConsumedStateMap *ElseOrMergeStates = new ConsumedStateMap(*CurrStates);

Perhaps not for this patch, but I'd feel more comfortable if
ConsumeStateMap was using an OwnedPtr instead of naked pointers.  It
would fix the memory leak that seems to be happening (items
successfully added to the state maps array do not seem to ever be
freed).

>
> -  bool HasElse = Terminator->getElse() != NULL;
> +  std::pair<unsigned, unsigned> TestRange =
> +    Visitor.getTestRange(Terminator->getCond());
>
> -  ConsumedStateMap *ElseOrMergeStates = new ConsumedStateMap(*CurrStates);
> +  Terminator->getCond()->dump();

I think this was accidentally left in and should be removed.

>
> -  if (Visitor.IsUsefulConditional) {
> -    ConsumedState VarState = CurrStates->getState(Visitor.Test.Var);
> +  // When there are no valid tests the start and end index will both be 0.
> +  while (TestRange.first < TestRange.second) {
> +    const VarTestResult &Test = Visitor.TestedVars[TestRange.first++];
> +    ConsumedState VarState    = CurrStates->getState(Test.Var);

More excess whitespace.

>
>      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);
> +      WarningsHandler.warnUnnecessaryTest(Test.Var->getNameAsString(),
> +        stateToString(VarState), Test.Loc);
>      }
>
> -    if (Visitor.Test.UnconsumedInTrueBranch) {
> -      CurrStates->setState(Visitor.Test.Var, CS_Unconsumed);
> -      if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var, CS_Consumed);
> +    if (Test.TrueBranchState == CS_Unconsumed) {
> +      CurrStates->setState(Test.Var, CS_Unconsumed);
> +      if (HasElse) ElseOrMergeStates->setState(Test.Var, CS_Consumed);

The usual convention is for the body to follow on the subsequent line
instead of inline.

>      } else {
> -      CurrStates->setState(Visitor.Test.Var, CS_Consumed);
> -      if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var, CS_Unconsumed);
> +      CurrStates->setState(Test.Var, CS_Consumed);
> +      if (HasElse) ElseOrMergeStates->setState(Test.Var, CS_Unconsumed);

Same here.

>      }
>    }
>
> @@ -773,7 +813,7 @@
>      if (const IfStmt *Terminator =
>        dyn_cast_or_null<IfStmt>(CurrBlock->getTerminator().getStmt())) {
>
> -      splitState(CurrBlock, Terminator);
> +      splitState(Visitor, CurrBlock, Terminator);
>        CurrStates = NULL;
>
>      } else if (CurrBlock->succ_size() > 1) {
>

~Aaron

On Thu, Aug 22, 2013 at 7:27 PM, Christian Wailes
<chriswailes at google.com> wrote:
> Hi dblaikie, delesley, aaron.ballman,
>
> The TestedVarsVisitor was folded into the ConsumedStmtVisitor.  The VarTestResult class was updated to allow these changes.  As well, the PropagationInfo class was updated for the same reasons.
>
> http://llvm-reviews.chandlerc.com/D1480
>
> Files:
>   include/clang/Analysis/Analyses/Consumed.h
>   lib/Analysis/Consumed.cpp



More information about the cfe-commits mailing list