[PATCH] Mostly correct conditional handling for Consumed analysis
Christian Wailes
chriswailes at google.com
Mon Aug 26 11:23:06 PDT 2013
If you are referring to the handling of the ConsumedStateMap objects, these
are guaranteed to be freed. A map can either be passed to another block
(and re-used) via CurrStates, passed to another block using the BlockInfo
manager, or merged into an existing block. The last case is the one where
we need to free a map. This occurs in the `run` method and the end of
`splitState` via a call to overloaded `addInfo`. The last existing map is
always deleted as the second to last statement in `run`.
There are responses to your comments bellow, and I am submitting a patch
with fixes now.
- Chris
On Sat, Aug 24, 2013 at 8:03 AM, Aaron Ballman <aaron.ballman at gmail.com>wrote:
> Mostly nits about formatting and whitespace, but my biggest concern
> has to do with memory leaks involving naked pointers (not really
> introduced in this patch). Also, there is an conversion to bool
> operator that kind of scares me (we don't have access to explicit
> operator conversions in C++11 yet, otherwise I would simply suggest
> marking that as explicit).
>
> ~Aaron
>
> > Index: include/clang/Analysis/Analyses/Consumed.h
> > ===================================================================
> > --- include/clang/Analysis/Analyses/Consumed.h
> > +++ include/clang/Analysis/Analyses/Consumed.h
> > @@ -24,7 +24,9 @@
> >
> > namespace clang {
> > namespace consumed {
> > -
> > +
> > + class ConsumedStmtVisitor;
> > +
> > typedef SmallVector<PartialDiagnosticAt, 1> OptionalNotes;
> > typedef std::pair<PartialDiagnosticAt, OptionalNotes> DelayedDiag;
> > typedef std::list<DelayedDiag> DiagList;
> > @@ -38,7 +40,7 @@
> > /// \brief Emit the warnings and notes left by the analysis.
> > virtual void emitDiagnostics() {}
> >
> > - /// Warn about unnecessary-test errors.
> > + /// \brief Warn about unnecessary-test errors.
> > /// \param VariableName -- The name of the variable that holds the
> unique
> > /// value.
> > ///
> > @@ -47,7 +49,7 @@
> > StringRef VariableState,
> > SourceLocation Loc) {}
> >
> > - /// Warn about use-while-consumed errors.
> > + /// \brief Warn about use-while-consumed errors.
> > /// \param MethodName -- The name of the method that was incorrectly
> > /// invoked.
> > ///
> > @@ -55,7 +57,7 @@
> > virtual void warnUseOfTempWhileConsumed(StringRef MethodName,
> > SourceLocation Loc) {}
> >
> > - /// Warn about use-in-unknown-state errors.
> > + /// \brief Warn about use-in-unknown-state errors.
> > /// \param MethodName -- The name of the method that was incorrectly
> > /// invoked.
> > ///
> > @@ -63,7 +65,7 @@
> > virtual void warnUseOfTempInUnknownState(StringRef MethodName,
> > SourceLocation Loc) {}
> >
> > - /// Warn about use-while-consumed errors.
> > + /// \brief Warn about use-while-consumed errors.
> > /// \param MethodName -- The name of the method that was incorrectly
> > /// invoked.
> > ///
> > @@ -75,7 +77,7 @@
> > StringRef VariableName,
> > SourceLocation Loc) {}
> >
> > - /// Warn about use-in-unknown-state errors.
> > + /// \brief Warn about use-in-unknown-state errors.
> > /// \param MethodName -- The name of the method that was incorrectly
> > /// invoked.
> > ///
> > @@ -90,7 +92,7 @@
> >
> > enum ConsumedState {
> > // No state information for the given variable.
> > - 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.
>
> >
> > CS_Unknown,
> > CS_Unconsumed,
> > @@ -104,18 +106,35 @@
> >
> > protected:
> >
> > + bool Reachable;
> > + const Stmt *From;
> > MapType Map;
> >
> > public:
> > + ConsumedStateMap() : Reachable(true), From(NULL) {}
> > + ConsumedStateMap(const ConsumedStateMap &Other)
> > + : Reachable(Other.Reachable), From(Other.From), Map(Other.Map) {}
> > +
> > /// \brief Get the consumed state of a given variable.
> > ConsumedState getState(const VarDecl *Var);
> >
> > /// \brief Merge this state map with another map.
> > void intersect(const ConsumedStateMap *Other);
> >
> > + /// \brief Return true if this block is reachable.
> > + bool isReachable();
>
> This method should be const and can be inlined.
>
> > +
> > /// \brief Mark all variables as unknown.
> > void makeUnknown();
> >
> > + /// \brief Mark the block as unreachable.
> > + void markUnreachable();
> > +
> > + /// \brief Set the source for a decision about the branching of
> states.
> > + /// \param Source -- The statement that was the origin of a
> branching
> > + /// decision.
> > + void setSource(const Stmt *Source);
>
> This one can be inlined as well.
>
> > +
> > /// \brief Set the consumed state of a given variable.
> > void setState(const VarDecl *Var, ConsumedState State);
> >
> > @@ -144,18 +163,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 +176,8 @@
> > CacheMapType ConsumableTypeCache;
> >
> > bool hasConsumableAttributes(const CXXRecordDecl *RD);
> > - void splitState(const CFGBlock *CurrBlock, const IfStmt
> *Terminator);
> > + bool splitState(const CFGBlock *CurrBlock,
> > + const ConsumedStmtVisitor &Visitor);
>
> Formatting for the Visitor.
>
Again, this isn't showing up in my patch or my editor. Are you viewing the
patch with non-fixed-width fonts?
>
> >
> > public:
> >
> > Index: lib/Analysis/Consumed.cpp
> > ===================================================================
> > --- lib/Analysis/Consumed.cpp
> > +++ lib/Analysis/Consumed.cpp
> > @@ -30,8 +30,13 @@
> > #include "llvm/ADT/SmallVector.h"
> > #include "llvm/Support/raw_ostream.h"
> >
> > +// TODO: Correctly identify unreachable blocks when chaining boolean
> operators.
> > +// TODO: Warn about unreachable code.
> > +// TODO: Switch to using a bitmap to track unreachable blocks.
> > // 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.
> > @@ -47,6 +52,29 @@
> > // Key method definition
> > ConsumedWarningsHandlerBase::~ConsumedWarningsHandlerBase() {}
> >
> > +static ConsumedState invertConsumedUnconsumed(ConsumedState State) {
> > + switch (State) {
> > + case CS_Unconsumed:
> > + return CS_Consumed;
> > + case CS_Consumed:
> > + return CS_Unconsumed;
> > + case CS_None:
> > + case CS_Unknown:
> > + llvm_unreachable("invalid enum");
> > + }
> > +}
> > +
> > +static bool isKnownState(ConsumedState State) {
> > + switch (State) {
> > + case CS_Unconsumed:
> > + case CS_Consumed:
> > + return true;
> > + case CS_None:
> > + case CS_Unknown:
> > + return false;
> > + }
> > +}
> > +
> > static bool isTestingFunction(const FunctionDecl *FunDecl) {
> > return FunDecl->hasAttr<TestsUnconsumedAttr>();
> > }
> > @@ -69,40 +97,143 @@
> > }
> >
> > namespace {
> > -class ConsumedStmtVisitor : public
> ConstStmtVisitor<ConsumedStmtVisitor> {
> > +struct VarTestResult {
> > + const VarDecl *Var;
> > + ConsumedState TestsFor;
> > +};
> > +} // end anonymous::VarTestResult
> > +
> > +namespace clang {
> > +namespace consumed {
> > +
> > +enum EffectiveOp {
> > + EO_And,
> > + EO_Or
> > +};
> > +
> > +class PropagationInfo {
> > + enum {
> > + IT_None,
> > + IT_State,
> > + IT_Test,
> > + IT_BinTest,
> > + IT_Var
> > + } InfoType;
> >
> > - union PropagationUnion {
> > + union {
> > ConsumedState State;
> > + VarTestResult Test;
> > const VarDecl *Var;
> > + struct {
> > + const BinaryOperator *Source;
> > + EffectiveOp EOp;
> > + VarTestResult LTest;
> > + VarTestResult RTest;
> > + } BinTest;
> > };
> >
> > - class PropagationInfo {
> > - PropagationUnion StateOrVar;
> > +public:
> > + PropagationInfo() : InfoType(IT_None) {}
> >
> > - public:
> > - bool IsVar;
> > + PropagationInfo(const VarTestResult &Test) : InfoType(IT_Test),
> Test(Test) {}
> > + PropagationInfo(const VarDecl *Var, ConsumedState TestsFor)
> > + : InfoType(IT_Test) {
> >
> > - PropagationInfo() : IsVar(false) {
> > - StateOrVar.State = consumed::CS_None;
> > - }
> > + Test.Var = Var;
>
> Whitespace
>
> > + Test.TestsFor = TestsFor;
> > + }
> > +
> > + PropagationInfo(const BinaryOperator *Source, EffectiveOp EOp,
> > + const VarTestResult <est, const VarTestResult
> &RTest)
> > + : InfoType(IT_BinTest) {
> >
> > - PropagationInfo(ConsumedState State) : IsVar(false) {
> > - StateOrVar.State = State;
> > - }
> > + BinTest.Source = Source;
> > + BinTest.EOp = EOp;
> > + BinTest.LTest = LTest;
> > + BinTest.RTest = RTest;
> > + }
> > +
> > + PropagationInfo(const BinaryOperator *Source, EffectiveOp EOp,
> > + const VarDecl *LVar, ConsumedState LTestsFor,
> > + const VarDecl *RVar, ConsumedState RTestsFor)
>
> Code formatting
>
Same comment as above concerning fonts.
>
> > + : InfoType(IT_BinTest) {
> >
> > - PropagationInfo(const VarDecl *Var) : IsVar(true) {
> > - StateOrVar.Var = Var;
> > - }
> > + BinTest.Source = Source;
> > + BinTest.EOp = EOp;
> > + BinTest.LTest.Var = LVar;
> > + BinTest.LTest.TestsFor = LTestsFor;
> > + BinTest.RTest.Var = RVar;
> > + BinTest.RTest.TestsFor = RTestsFor;
>
> Hopefully this looks better in the editor than it does in my mail
> client. ;-) I don't think there's a rule against it, but I don't
> think many places in the code base attempt to line up assignments.
> The only place I've ever really seen this is with case statement
> bodies, and even there it's not consistent.
>
> > + }
> > +
> > + PropagationInfo(ConsumedState State) : InfoType(IT_State),
> State(State) {}
> > + PropagationInfo(const VarDecl *Var) : InfoType(IT_Var), Var(Var) {}
> > +
> > + const ConsumedState & getState() const {
> > + assert(InfoType == IT_State);
> > + return State;
> > + }
> > +
> > + const VarTestResult & getTest() const {
> > + assert(InfoType == IT_Test);
> > + return Test;
> > + }
> > +
> > + const VarTestResult & getLTest() const {
> > + assert(InfoType == IT_BinTest);
> > + return BinTest.LTest;
> > + }
> > +
> > + const VarTestResult & getRTest() const {
> > + assert(InfoType == IT_BinTest);
> > + return BinTest.RTest;
> > + }
> > +
> > + const VarDecl * getVar() const {
> > + assert(InfoType == IT_Var);
> > + return Var;
> > + }
> > +
> > + EffectiveOp testEffectiveOp() const {
> > + assert(InfoType == IT_BinTest);
> > + return BinTest.EOp;
> > + }
> > +
> > + const BinaryOperator * testSourceNode() const {
> > + assert(InfoType == IT_BinTest);
> > + return BinTest.Source;
> > + }
> > +
> > + operator bool() const { return InfoType != IT_None; }
>
> Implicit conversions to bool are kind of terrifying. This makes the
> class now assignable as an integer, which can lead to obscure bugs.
> I'd prefer to avoid this.
>
>
I think "is there information or not" is a pretty obvious test, so I would
like to go with David's suggestion and use LLVM_EXPLICIT if there aren't
any objections.
> > + bool isState() const { return InfoType == IT_State; }
> > + bool isTest() const { return InfoType == IT_Test; }
> > + bool isBinTest() const { return InfoType == IT_BinTest; }
> > + bool isVar() const { return InfoType == IT_Var; }
>
> Whitespace between the end of the parameter list and the const.
>
> > +
> > + PropagationInfo invertTest() const {
> > + assert(InfoType == IT_Test || InfoType == IT_BinTest);
> >
> > - ConsumedState getState() const { return StateOrVar.State; }
> > + if (InfoType == IT_Test) {
> > + return PropagationInfo(Test.Var,
> invertConsumedUnconsumed(Test.TestsFor));
> >
> > - const VarDecl * getVar() const { return IsVar ? StateOrVar.Var :
> NULL; }
> > - };
> > + } else if (InfoType == IT_BinTest) {
> > + return PropagationInfo(BinTest.Source,
> > + BinTest.EOp == EO_And ? EO_Or : EO_And,
> > + BinTest.LTest.Var,
> invertConsumedUnconsumed(BinTest.LTest.TestsFor),
> > + BinTest.RTest.Var,
> invertConsumedUnconsumed(BinTest.RTest.TestsFor));
> > + } else {
> > + return PropagationInfo();
> > + }
> > + }
> > +};
> > +
> > +class ConsumedStmtVisitor : public
> ConstStmtVisitor<ConsumedStmtVisitor> {
> >
> > 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;
> > @@ -112,10 +243,11 @@
> > const FunctionDecl *FunDecl,
> > const CallExpr *Call);
> > void forwardInfo(const Stmt *From, const Stmt *To);
> > + void handleTestingFunctionCall(const CallExpr *Call, const VarDecl
> *Var);
> > bool isLikeMoveAssignment(const CXXMethodDecl *MethodDecl);
> >
> > public:
> > -
> > +
> > void Visit(const Stmt *StmtNode);
> >
> > void VisitBinaryOperator(const BinaryOperator *BinOp);
> > @@ -131,12 +263,20 @@
> > void VisitUnaryOperator(const UnaryOperator *UOp);
> > void VisitVarDecl(const VarDecl *Var);
> >
> > - ConsumedStmtVisitor(AnalysisDeclContext &AC, ConsumedAnalyzer
> &Analyzer,
> > - ConsumedStateMap *StateMap)
> > - : AC(AC), Analyzer(Analyzer), StateMap(StateMap) {}
> > -
> > - void reset() {
> > - PropagationMap.clear();
> > + ConsumedStmtVisitor(AnalysisDeclContext &AC, ConsumedAnalyzer
> &Analyzer)
> > + : AC(AC), Analyzer(Analyzer), StateMap(NULL) {}
> > +
> > + PropagationInfo getInfo(const Stmt *StmtNode) const {
> > + ConstInfoEntry Entry = PropagationMap.find(StmtNode);
> > +
> > + if (Entry != PropagationMap.end())
> > + return Entry->second;
> > + else
> > + return PropagationInfo();
> > + }
> > +
> > + void reset(ConsumedStateMap *NewStateMap) {
> > + StateMap = NewStateMap;
> > }
> > };
> >
> > @@ -148,7 +288,7 @@
> >
> > if (!FunDecl->hasAttr<CallableWhenUnconsumedAttr>()) return;
> >
> > - if (PInfo.IsVar) {
> > + if (PInfo.isVar()) {
> > const VarDecl *Var = PInfo.getVar();
> >
> > switch (StateMap->getState(Var)) {
> > @@ -191,9 +331,24 @@
> > void ConsumedStmtVisitor::forwardInfo(const Stmt *From, const Stmt *To)
> {
> > InfoEntry Entry = PropagationMap.find(From);
> >
> > - if (Entry != PropagationMap.end()) {
> > - PropagationMap.insert(PairType(To, PropagationInfo(Entry->second)));
> > + if (Entry != PropagationMap.end())
> > + PropagationMap.insert(PairType(To, Entry->second));
> > +}
> > +
> > +void ConsumedStmtVisitor::handleTestingFunctionCall(const CallExpr
> *Call,
> > + const VarDecl
> *Var) {
> > +
> > + ConsumedState VarState = StateMap->getState(Var);
> > +
> > + if (VarState != CS_Unknown) {
> > + SourceLocation CallLoc = Call->getExprLoc();
> > +
> > + if (!CallLoc.isMacroID())
> > +
> Analyzer.WarningsHandler.warnUnnecessaryTest(Var->getNameAsString(),
> > + stateToString(VarState), CallLoc);
> > }
> > +
> > + PropagationMap.insert(PairType(Call, PropagationInfo(Var,
> CS_Unconsumed)));
> > }
> >
> > bool ConsumedStmtVisitor::isLikeMoveAssignment(
> > @@ -205,8 +360,43 @@
> >
> MethodDecl->getParamDecl(0)->getType()->isRValueReferenceType());
> > }
> >
> > +void ConsumedStmtVisitor::Visit(const Stmt *StmtNode) {
> > +
> > + ConstStmtVisitor<ConsumedStmtVisitor>::Visit(StmtNode);
> > +
> > + for (Stmt::const_child_iterator CI = StmtNode->child_begin(),
> > + CE = StmtNode->child_end(); CI != CE; ++CI) {
> > +
> > + PropagationMap.erase(*CI);
> > + }
> > +}
> > +
> > void ConsumedStmtVisitor::VisitBinaryOperator(const BinaryOperator
> *BinOp) {
> > switch (BinOp->getOpcode()) {
> > + case BO_LAnd:
> > + case BO_LOr : {
> > + InfoEntry LEntry = PropagationMap.find(BinOp->getLHS()),
> > + REntry = PropagationMap.find(BinOp->getRHS());
> > +
> > + VarTestResult LTest, RTest;
> > +
> > + if (LEntry != PropagationMap.end() && LEntry->second.isTest())
> > + LTest = LEntry->second.getTest();
> > + else
> > + LTest.Var = NULL;
> > +
> > + if (REntry != PropagationMap.end() && REntry->second.isTest())
> > + RTest = REntry->second.getTest();
> > + else
> > + RTest.Var = NULL;
> > +
> > + if (!(LTest.Var == NULL && RTest.Var == NULL))
> > + PropagationMap.insert(PairType(BinOp, PropagationInfo(BinOp,
> > + static_cast<EffectiveOp>(BinOp->getOpcode() == BO_LOr), LTest,
> RTest)));
> > +
> > + break;
> > + }
> > +
> > case BO_PtrMemD:
> > case BO_PtrMemI:
> > forwardInfo(BinOp->getLHS(), BinOp);
> > @@ -217,16 +407,6 @@
> > }
> > }
> >
> > -void ConsumedStmtVisitor::Visit(const Stmt *StmtNode) {
> > - ConstStmtVisitor<ConsumedStmtVisitor>::Visit(StmtNode);
> > -
> > - for (Stmt::const_child_iterator CI = StmtNode->child_begin(),
> > - CE = StmtNode->child_end(); CI != CE; ++CI) {
> > -
> > - PropagationMap.erase(*CI);
> > - }
> > -}
> > -
> > void ConsumedStmtVisitor::VisitCallExpr(const CallExpr *Call) {
> > if (const FunctionDecl *FunDecl =
> > dyn_cast_or_null<FunctionDecl>(Call->getDirectCallee())) {
> > @@ -250,7 +430,7 @@
> >
> > InfoEntry Entry = PropagationMap.find(Call->getArg(Index));
> >
> > - if (Entry == PropagationMap.end() || !Entry->second.IsVar) {
> > + if (Entry == PropagationMap.end() || !Entry->second.isVar()) {
> > continue;
> > }
> >
> > @@ -274,10 +454,7 @@
> > }
> >
> > void ConsumedStmtVisitor::VisitCastExpr(const CastExpr *Cast) {
> > - InfoEntry Entry = PropagationMap.find(Cast->getSubExpr());
> > -
> > - if (Entry != PropagationMap.end())
> > - PropagationMap.insert(PairType(Cast, Entry->second));
> > + forwardInfo(Cast->getSubExpr(), Cast);
> > }
> >
> > void ConsumedStmtVisitor::VisitCXXConstructExpr(const CXXConstructExpr
> *Call) {
> > @@ -298,7 +475,7 @@
> > PropagationInfo PInfo =
> > PropagationMap.find(Call->getArg(0))->second;
> >
> > - if (PInfo.IsVar) {
> > + if (PInfo.isVar()) {
> > const VarDecl* Var = PInfo.getVar();
> >
> > PropagationMap.insert(PairType(Call,
> > @@ -336,8 +513,10 @@
> >
> > checkCallability(PInfo, MethodDecl, Call);
> >
> > - if (PInfo.IsVar) {
> > - if (MethodDecl->hasAttr<ConsumesAttr>())
> > + if (PInfo.isVar()) {
> > + if (isTestingFunction(MethodDecl))
> > + handleTestingFunctionCall(Call, PInfo.getVar());
> > + else if (MethodDecl->hasAttr<ConsumesAttr>())
> > StateMap->setState(PInfo.getVar(), consumed::CS_Consumed);
> > else if (!MethodDecl->isConst())
> > StateMap->setState(PInfo.getVar(), consumed::CS_Unknown);
> > @@ -367,7 +546,7 @@
> > LPInfo = LEntry->second;
> > RPInfo = REntry->second;
> >
> > - if (LPInfo.IsVar && RPInfo.IsVar) {
> > + if (LPInfo.isVar() && RPInfo.isVar()) {
> > StateMap->setState(LPInfo.getVar(),
> > StateMap->getState(RPInfo.getVar()));
> >
> > @@ -375,12 +554,12 @@
> >
> > PropagationMap.insert(PairType(Call, LPInfo));
> >
> > - } else if (LPInfo.IsVar && !RPInfo.IsVar) {
> > + } else if (LPInfo.isVar() && !RPInfo.isVar()) {
> > StateMap->setState(LPInfo.getVar(), RPInfo.getState());
> >
> > PropagationMap.insert(PairType(Call, LPInfo));
> >
> > - } else if (!LPInfo.IsVar && RPInfo.IsVar) {
> > + } else if (!LPInfo.isVar() && RPInfo.isVar()) {
> > PropagationMap.insert(PairType(Call,
> > PropagationInfo(StateMap->getState(RPInfo.getVar()))));
> >
> > @@ -395,7 +574,7 @@
> >
> > LPInfo = LEntry->second;
> >
> > - if (LPInfo.IsVar) {
> > + if (LPInfo.isVar()) {
> > StateMap->setState(LPInfo.getVar(), consumed::CS_Unknown);
> >
> > PropagationMap.insert(PairType(Call, LPInfo));
> > @@ -410,7 +589,7 @@
> >
> > RPInfo = REntry->second;
> >
> > - if (RPInfo.IsVar) {
> > + if (RPInfo.isVar()) {
> > const VarDecl *Var = RPInfo.getVar();
> >
> > PropagationMap.insert(PairType(Call,
> > @@ -434,14 +613,16 @@
> >
> > checkCallability(PInfo, FunDecl, Call);
> >
> > - if (PInfo.IsVar) {
> > - if (FunDecl->hasAttr<ConsumesAttr>()) {
> > - // Handle consuming operators.
> > + if (PInfo.isVar()) {
> > + if (isTestingFunction(FunDecl)) {
> > + handleTestingFunctionCall(Call, PInfo.getVar());
> > +
> > + } else if (FunDecl->hasAttr<ConsumesAttr>()) {
> > StateMap->setState(PInfo.getVar(), consumed::CS_Consumed);
> > +
> > } else if (const CXXMethodDecl *MethodDecl =
> > - dyn_cast_or_null<CXXMethodDecl>(FunDecl)) {
> > + dyn_cast_or_null<CXXMethodDecl>(FunDecl)) {
> >
> > - // Handle non-constant member operators.
> > if (!MethodDecl->isConst())
> > StateMap->setState(PInfo.getVar(), consumed::CS_Unknown);
> > }
> > @@ -482,11 +663,21 @@
> > }
> >
> > 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));
> > + InfoEntry Entry =
> PropagationMap.find(UOp->getSubExpr()->IgnoreParens());
> > + if (Entry == PropagationMap.end()) return;
> > +
> > + switch (UOp->getOpcode()) {
> > + case UO_AddrOf:
> > + PropagationMap.insert(PairType(UOp, Entry->second));
> > + break;
> > +
> > + case UO_LNot:
> > + if (Entry->second.isTest() || Entry->second.isBinTest())
> > + PropagationMap.insert(PairType(UOp, Entry->second.invertTest()));
> > + break;
> > +
> > + default:
> > + break;
> > }
> > }
> >
> > @@ -495,74 +686,97 @@
> > PropagationInfo PInfo =
> > PropagationMap.find(Var->getInit())->second;
> >
> > - StateMap->setState(Var, PInfo.IsVar ?
> > + StateMap->setState(Var, PInfo.isVar() ?
> > StateMap->getState(PInfo.getVar()) : PInfo.getState());
> > }
> > }
> > -} // end anonymous::ConsumedStmtVisitor
> > +}} // end clang::consumed::ConsumedStmtVisitor
>
> Two curly braces on the same line.
>
I've seen this happen in other places in the Clang codebase. It seems
better than wasting a newline to me.
>
> >
> > -namespace {
> > +namespace clang {
> > +namespace consumed {
> >
> > -// TODO: Handle variable definitions, e.g. bool valid = x.isValid();
> > -// if (valid) ...; (Deferred)
> > -class TestedVarsVisitor : public RecursiveASTVisitor<TestedVarsVisitor>
> {
> > -
> > - bool Invert;
> > - SourceLocation CurrTestLoc;
> > -
> > - ConsumedStateMap *StateMap;
> > -
> > -public:
> > - bool IsUsefulConditional;
> > - VarTestResult Test;
> > +void splitVarStateForIf(const IfStmt * IfNode, const VarTestResult
> &Test,
> > + ConsumedStateMap *ThenStates,
> > + ConsumedStateMap *ElseStates) {
> > +
> > + ConsumedState VarState = ThenStates->getState(Test.Var);
> >
> > - TestedVarsVisitor(ConsumedStateMap *StateMap) : Invert(false),
> > - StateMap(StateMap), IsUsefulConditional(false) {}
> > + if (VarState == CS_Unknown) {
> > + ThenStates->setState(Test.Var, Test.TestsFor);
> > + if (ElseStates)
> > + ElseStates->setState(Test.Var,
> invertConsumedUnconsumed(Test.TestsFor));
> >
> > - 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;
> > - }
> > + } else if (VarState == invertConsumedUnconsumed(Test.TestsFor)) {
> > + ThenStates->markUnreachable();
> >
> > - IsUsefulConditional = false;
> > + } else if (VarState == Test.TestsFor && ElseStates) {
> > + ElseStates->markUnreachable();
> > }
> > -
> > - 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);
> > +void splitVarStateForIfBinOp(const PropagationInfo &PInfo,
> > + ConsumedStateMap *ThenStates, ConsumedStateMap *ElseStates) {
> >
> > - return true;
> > -}
> > -
> > -bool TestedVarsVisitor::VisitUnaryOperator(UnaryOperator *UnaryOp) {
> > - if (UnaryOp->getOpcode() == UO_LNot) {
> > - Invert = true;
> > - TraverseStmt(UnaryOp->getSubExpr());
> > -
> > - } else {
> > - IsUsefulConditional = false;
> > + const VarTestResult <est = PInfo.getLTest(),
> > + &RTest = PInfo.getRTest();
> > +
> > + ConsumedState LState = LTest.Var ? ThenStates->getState(LTest.Var) :
> CS_None,
> > + RState = RTest.Var ? ThenStates->getState(RTest.Var) :
> CS_None;
> > +
> > + if (LTest.Var) {
> > + if (PInfo.testEffectiveOp() == EO_And) {
> > + if (LState == CS_Unknown) {
> > + ThenStates->setState(LTest.Var, LTest.TestsFor);
> > +
> > + } else if (LState == invertConsumedUnconsumed(LTest.TestsFor)) {
> > + ThenStates->markUnreachable();
> > +
> > + } else if (LState == LTest.TestsFor && isKnownState(RState)) {
> > + if (RState == RTest.TestsFor) {
> > + if (ElseStates)
> > + ElseStates->markUnreachable();
> > + } else {
> > + ThenStates->markUnreachable();
> > + }
> > + }
> > +
> > + } else {
> > + if (LState == CS_Unknown && ElseStates) {
> > + ElseStates->setState(LTest.Var,
> > + invertConsumedUnconsumed(LTest.TestsFor));
> > +
> > + } else if (LState == LTest.TestsFor && ElseStates) {
> > + ElseStates->markUnreachable();
> > +
> > + } else if (LState == invertConsumedUnconsumed(LTest.TestsFor) &&
> > + isKnownState(RState)) {
> > +
> > + if (RState == RTest.TestsFor) {
> > + if (ElseStates)
> > + ElseStates->markUnreachable();
> > + } else {
> > + ThenStates->markUnreachable();
> > + }
> > + }
> > + }
> > }
> >
> > - return false;
> > + if (RTest.Var) {
> > + if (PInfo.testEffectiveOp() == EO_And) {
> > + if (RState == CS_Unknown)
> > + ThenStates->setState(RTest.Var, RTest.TestsFor);
> > + else if (RState == invertConsumedUnconsumed(RTest.TestsFor))
> > + ThenStates->markUnreachable();
> > +
> > + } else if (ElseStates) {
> > + if (RState == CS_Unknown)
> > + ElseStates->setState(RTest.Var,
> > + invertConsumedUnconsumed(RTest.TestsFor));
> > + else if (RState == RTest.TestsFor)
> > + ElseStates->markUnreachable();
> > + }
> > + }
> > }
> > -} // end anonymouse::TestedVarsVisitor
> > -
> > -namespace clang {
> > -namespace consumed {
> >
> > void ConsumedBlockInfo::addInfo(const CFGBlock *Block,
> > ConsumedStateMap *StateMap,
> > @@ -625,29 +839,44 @@
> > void ConsumedStateMap::intersect(const ConsumedStateMap *Other) {
> > ConsumedState LocalState;
> >
> > + if (this->From && this->From == Other->From && !Other->Reachable) {
> > + this->markUnreachable();
> > + return;
> > + }
> > +
> > for (MapType::const_iterator DMI = Other->Map.begin(),
> > DME = Other->Map.end(); DMI != DME; ++DMI) {
> >
> > LocalState = this->getState(DMI->first);
> >
> > - if (LocalState != CS_None && LocalState != DMI->second)
> > - setState(DMI->first, CS_Unknown);
> > + if (LocalState == CS_None) continue;
>
> Body should be on a new line.
>
> > +
> > + if (LocalState != DMI->second)
> > + Map[DMI->first] = CS_Unknown;
> > }
> > }
> >
> > +bool ConsumedStateMap::isReachable() {
> > + return Reachable;
> > +}
>
> Function should be const and inline.
>
> > +
> > +void ConsumedStateMap::markUnreachable() {
> > + this->Reachable = false;
> > + Map.clear();
> > +}
> > +
> > void ConsumedStateMap::makeUnknown() {
> > - PairType Pair;
> > -
> > for (MapType::const_iterator DMI = Map.begin(), DME = Map.end(); DMI
> != DME;
> > ++DMI) {
> >
> > - Pair = *DMI;
> > -
> > - Map.erase(Pair.first);
> > - Map.insert(PairType(Pair.first, CS_Unknown));
> > + Map[DMI->first] = CS_Unknown;
> > }
> > }
> >
> > +void ConsumedStateMap::setSource(const Stmt *Source) {
> > + this->From = Source;
> > +}
> > +
> > void ConsumedStateMap::setState(const VarDecl *Var, ConsumedState
> State) {
> > Map[Var] = State;
> > }
> > @@ -694,42 +923,84 @@
> > 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) {
> > -
> > - TestedVarsVisitor Visitor(CurrStates);
> > - Visitor.TraverseStmt(const_cast<Expr*>(Terminator->getCond()));
> > -
> > - bool HasElse = Terminator->getElse() != NULL;
> > +bool ConsumedAnalyzer::splitState(const CFGBlock *CurrBlock,
> > + const ConsumedStmtVisitor &Visitor) {
>
> Formatting
>
Non-fixed-width font issue.
>
> >
> > - ConsumedStateMap *ElseOrMergeStates = new
> ConsumedStateMap(*CurrStates);
> > + ConsumedStateMap *FalseStates = new ConsumedStateMap(*CurrStates);
>
> I am still concerted about the leaks with naked pointers.
>
> > + PropagationInfo PInfo;
> >
> > - 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();
> > +
> > + PInfo = Visitor.getInfo(Cond);
> > + if (!PInfo && isa<BinaryOperator>(Cond))
> > + PInfo = Visitor.getInfo(cast<BinaryOperator>(Cond)->getRHS());
> >
> > - if (Visitor.Test.UnconsumedInTrueBranch) {
> > - CurrStates->setState(Visitor.Test.Var, CS_Unconsumed);
> > - if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var,
> CS_Consumed);
> > + 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);
> >
> > } else {
> > - CurrStates->setState(Visitor.Test.Var, CS_Consumed);
> > - if (HasElse) ElseOrMergeStates->setState(Visitor.Test.Var,
> CS_Unconsumed);
> > + return false;
> > + }
> > +
> > + } 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;
> > + }
> > + }
> > +
> > + 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;
> > + }
> > +
> > 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);
> > + if (*++SI) BlockInfo.addInfo(*SI, FalseStates);
> > +
> > + CurrStates = NULL;
> > + return true;
> > }
> >
> > void ConsumedAnalyzer::run(AnalysisDeclContext &AC) {
> > @@ -742,6 +1013,7 @@
> > PostOrderCFGView *SortedGraph = AC.getAnalysis<PostOrderCFGView>();
> >
> > CurrStates = new ConsumedStateMap();
> > + ConsumedStmtVisitor Visitor(AC, *this);
> >
> > // Visit all of the function's basic blocks.
> > for (PostOrderCFGView::iterator I = SortedGraph->begin(),
> > @@ -752,9 +1024,19 @@
> >
> > if (CurrStates == NULL)
> > CurrStates = BlockInfo.getInfo(CurrBlock);
> > -
> > - ConsumedStmtVisitor Visitor(AC, *this, CurrStates);
> > -
> > +
> > + if (!CurrStates) {
> > + CurrStates = NULL;
> > + continue;
> > +
> > + } else if (!CurrStates->isReachable()) {
> > + delete CurrStates;
> > + CurrStates = NULL;
> > + continue;
> > + }
> > +
> > + Visitor.reset(CurrStates);
> > +
> > // Visit all of the basic block's statements.
> > for (CFGBlock::const_iterator BI = CurrBlock->begin(),
> > BE = CurrBlock->end(); BI != BE; ++BI) {
> > @@ -770,36 +1052,34 @@
> > }
> > }
> >
> > - if (const IfStmt *Terminator =
> > - dyn_cast_or_null<IfStmt>(CurrBlock->getTerminator().getStmt())) {
> > + // TODO: Handle other forms of branching with precision, including
> while-
> > + // and for-loops. (Deferred)
> > + if (!splitState(CurrBlock, Visitor)) {
> > + CurrStates->setSource(NULL);
> >
> > - splitState(CurrBlock, Terminator);
> > - CurrStates = NULL;
> > -
> > - } else if (CurrBlock->succ_size() > 1) {
> > - CurrStates->makeUnknown();
> > -
> > - bool OwnershipTaken = false;
> > -
> > - for (CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin(),
> > - SE = CurrBlock->succ_end(); SI != SE; ++SI) {
> > + if (CurrBlock->succ_size() > 1) {
> > + CurrStates->makeUnknown();
> > +
> > + bool OwnershipTaken = false;
> >
> > - if (*SI) BlockInfo.addInfo(*SI, CurrStates, OwnershipTaken);
> > + for (CFGBlock::const_succ_iterator SI = CurrBlock->succ_begin(),
> > + SE = CurrBlock->succ_end(); SI != SE; ++SI) {
> > +
> > + if (*SI) BlockInfo.addInfo(*SI, CurrStates, OwnershipTaken);
> > + }
> > +
> > + if (!OwnershipTaken)
> > + delete CurrStates;
> > +
> > + CurrStates = NULL;
> > +
> > + } else if (CurrBlock->succ_size() == 1 &&
> > + (*CurrBlock->succ_begin())->pred_size() > 1) {
> > +
> > + BlockInfo.addInfo(*CurrBlock->succ_begin(), CurrStates);
> > + CurrStates = NULL;
> > }
> > -
> > - if (!OwnershipTaken)
> > - delete CurrStates;
> > -
> > - CurrStates = NULL;
> > -
> > - } else if (CurrBlock->succ_size() == 1 &&
> > - (*CurrBlock->succ_begin())->pred_size() > 1) {
> > -
> > - BlockInfo.addInfo(*CurrBlock->succ_begin(), CurrStates);
> > - CurrStates = NULL;
> > }
> > -
> > - Visitor.reset();
> > } // End of block iterator.
> >
> > // Delete the last existing state map.
> > Index: test/SemaCXX/warn-consumed-analysis-strict.cpp
> > ===================================================================
> > --- test/SemaCXX/warn-consumed-analysis-strict.cpp
> > +++ test/SemaCXX/warn-consumed-analysis-strict.cpp
> > @@ -4,6 +4,8 @@
> > #define CONSUMES __attribute__ ((consumes))
> > #define TESTS_UNCONSUMED __attribute__ ((tests_unconsumed))
> >
> > +#define TEST_VAR(Var) Var.isValid()
> > +
> > typedef decltype(nullptr) nullptr_t;
> >
> > template <typename T>
> > @@ -11,7 +13,7 @@
> > T var;
> >
> > public:
> > - ConsumableClass(void);
> > + ConsumableClass();
> > ConsumableClass(T val);
> > ConsumableClass(ConsumableClass<T> &other);
> > ConsumableClass(ConsumableClass<T> &&other);
> > @@ -26,20 +28,24 @@
> > template <typename U>
> > ConsumableClass<T>& operator=(ConsumableClass<U> &&other);
> >
> > - void operator*(void) const CALLABLE_WHEN_UNCONSUMED;
> > + void operator()(int a) CONSUMES;
> > + void operator*() const CALLABLE_WHEN_UNCONSUMED;
> > + void unconsumedCall() const CALLABLE_WHEN_UNCONSUMED;
> >
> > - bool isValid(void) const TESTS_UNCONSUMED;
> > + bool isValid() const TESTS_UNCONSUMED;
> > + operator bool() const TESTS_UNCONSUMED;
> > + bool operator!=(nullptr_t) const TESTS_UNCONSUMED;
> >
> > - void constCall(void) const;
> > - void nonconstCall(void);
> > + void constCall() const;
> > + void nonconstCall();
> >
> > - void consume(void) CONSUMES;
> > + void consume() CONSUMES;
> > };
> >
> > void baf0(ConsumableClass<int> &var);
> > void baf1(ConsumableClass<int> *var);
> >
> > -void testIfStmt(void) {
> > +void testIfStmt() {
> > ConsumableClass<int> var;
> >
> > if (var.isValid()) { // expected-warning {{unnecessary test. Variable
> 'var' is known to be in the 'consumed' state}}
> > @@ -51,27 +57,113 @@
> > }
> > }
> >
> > -void testConditionalMerge(void) {
> > - ConsumableClass<int> var;
> > +void testComplexConditionals() {
> > + ConsumableClass<int> var0, var1, var2;
> > +
> > + // Coerce all variables into the unknown state.
> > + baf0(var0);
> > + baf0(var1);
> > + baf0(var2);
> >
> > - if (var.isValid()) {// expected-warning {{unnecessary test. Variable
> 'var' is known to be in the 'consumed' state}}
> > + if (var0 && var1) {
> > + *var0;
> > + *var1;
> >
> > - // Empty
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > }
> >
> > - *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in an unknown state}}
> > + if (var0 || var1) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > + }
> >
> > - if (var.isValid()) {
> > - // Empty
> > + if (var0 && !var1) {
> > + *var0;
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> >
> > } else {
> > - // Empty
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > }
> >
> > - *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in an unknown state}}
> > + if (var0 || !var1) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1;
> > + }
> > +
> > + if (!var0 && !var1) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > + }
> > +
> > + if (!(var0 || var1)) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > + }
> > +
> > + if (!var0 || !var1) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > +
> > + } else {
> > + *var0;
> > + *var1;
> > + }
> > +
> > + if (!(var0 && var1)) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > +
> > + } else {
> > + *var0;
> > + *var1;
> > + }
> > +
> > + if (var0 && var1 && var2) {
> > + *var0;
> > + *var1;
> > + *var2;
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > + *var2; // expected-warning {{invocation of method 'operator*' on
> object 'var2' while it is in an unknown state}}
> > + }
> > +
> > +#if 0
> > + // FIXME: Get this test to pass.
> > + if (var0 || var1 || var2) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in an unknown state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in an unknown state}}
> > + *var2; // expected-warning {{invocation of method 'operator*' on
> object 'var2' while it is in an unknown state}}
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > + *var2; // expected-warning {{invocation of method 'operator*' on
> object 'var2' while it is in the 'consumed' state}}
> > + }
> > +#endif
> > }
> >
> > -void testCallingConventions(void) {
> > +void testCallingConventions() {
> > ConsumableClass<int> var(42);
> >
> > baf0(var);
> > @@ -82,14 +174,14 @@
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in an unknown state}}
> > }
> >
> > -void testMoveAsignmentish(void) {
> > +void testMoveAsignmentish() {
> > ConsumableClass<int> var;
> >
> > var = nullptr;
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in an unknown state}}
> > }
> >
> > -void testConstAndNonConstMemberFunctions(void) {
> > +void testConstAndNonConstMemberFunctions() {
> > ConsumableClass<int> var(42);
> >
> > var.constCall();
> > @@ -99,7 +191,15 @@
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in an unknown state}}
> > }
> >
> > -void testSimpleForLoop(void) {
> > +void testNoWarnTestFromMacroExpansion() {
> > + ConsumableClass<int> var(42);
> > +
> > + if (TEST_VAR(var)) {
> > + *var;
> > + }
> > +}
> > +
> > +void testSimpleForLoop() {
> > ConsumableClass<int> var;
> >
> > for (int i = 0; i < 10; ++i) {
> > @@ -109,7 +209,7 @@
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in an unknown state}}
> > }
> >
> > -void testSimpleWhileLoop(void) {
> > +void testSimpleWhileLoop() {
> > int i = 0;
> >
> > ConsumableClass<int> var;
> > Index: test/SemaCXX/warn-consumed-analysis.cpp
> > ===================================================================
> > --- test/SemaCXX/warn-consumed-analysis.cpp
> > +++ test/SemaCXX/warn-consumed-analysis.cpp
> > @@ -11,7 +11,7 @@
> > T var;
> >
> > public:
> > - ConsumableClass(void);
> > + ConsumableClass();
> > ConsumableClass(nullptr_t p) CONSUMES;
> > ConsumableClass(T val);
> > ConsumableClass(ConsumableClass<T> &other);
> > @@ -28,17 +28,17 @@
> > ConsumableClass<T>& operator=(ConsumableClass<U> &&other);
> >
> > void operator()(int a) CONSUMES;
> > - void operator*(void) const CALLABLE_WHEN_UNCONSUMED;
> > - void unconsumedCall(void) const CALLABLE_WHEN_UNCONSUMED;
> > + void operator*() const CALLABLE_WHEN_UNCONSUMED;
> > + void unconsumedCall() const CALLABLE_WHEN_UNCONSUMED;
> >
> > - bool isValid(void) const TESTS_UNCONSUMED;
> > + bool isValid() const TESTS_UNCONSUMED;
> > operator bool() const TESTS_UNCONSUMED;
> > bool operator!=(nullptr_t) const TESTS_UNCONSUMED;
> >
> > - void constCall(void) const;
> > - void nonconstCall(void);
> > + void constCall() const;
> > + void nonconstCall();
> >
> > - void consume(void) CONSUMES;
> > + void consume() CONSUMES;
> > };
> >
> > void baf0(const ConsumableClass<int> var);
> > @@ -47,7 +47,7 @@
> >
> > void baf3(ConsumableClass<int> &&var);
> >
> > -void testInitialization(void) {
> > +void testInitialization() {
> > ConsumableClass<int> var0;
> > ConsumableClass<int> var1 = ConsumableClass<int>();
> >
> > @@ -58,10 +58,10 @@
> >
> > if (var0.isValid()) {
> > *var0;
> > - *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > + *var1;
> >
> > } else {
> > - *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > }
> > }
> >
> > @@ -69,7 +69,7 @@
> > *ConsumableClass<int>(); // expected-warning {{invocation of method
> 'operator*' on a temporary object while it is in the 'consumed' state}}
> > }
> >
> > -void testSimpleRValueRefs(void) {
> > +void testSimpleRValueRefs() {
> > ConsumableClass<int> var0;
> > ConsumableClass<int> var1(42);
> >
> > @@ -82,39 +82,149 @@
> > *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > }
> >
> > -void testIfStmt(void) {
> > +void testIfStmt() {
> > ConsumableClass<int> var;
> >
> > if (var.isValid()) {
> > - // Empty
> > -
> > + *var;
> > } else {
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> > }
> >
> > if (!var.isValid()) {
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> > -
> > } else {
> > *var;
> > }
> >
> > if (var) {
> > // Empty
> > -
> > } else {
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> > }
> >
> > if (var != nullptr) {
> > // Empty
> > -
> > } else {
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> > }
> > }
> >
> > -void testCallingConventions(void) {
> > +void testComplexConditionals() {
> > + ConsumableClass<int> var0, var1, var2;
> > +
> > + if (var0 && var1) {
> > + *var0;
> > + *var1;
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > + }
> > +
> > + if (var0 || var1) {
> > + *var0;
> > + *var1;
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > + }
> > +
> > + if (var0 && !var1) {
> > + *var0;
> > + *var1;
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > + }
> > +
> > + if (var0 || !var1) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > +
> > + } else {
> > + *var0;
> > + *var1;
> > + }
> > +
> > + if (!var0 && !var1) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > +
> > + } else {
> > + *var0;
> > + *var1;
> > + }
> > +
> > + if (!var0 || !var1) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > +
> > + } else {
> > + *var0;
> > + *var1;
> > + }
> > +
> > + if (!(var0 && var1)) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > +
> > + } else {
> > + *var0;
> > + *var1;
> > + }
> > +
> > + if (!(var0 || var1)) {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > +
> > + } else {
> > + *var0;
> > + *var1;
> > + }
> > +
> > + if (var0 && var1 && var2) {
> > + *var0;
> > + *var1;
> > + *var2;
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > + *var2; // expected-warning {{invocation of method 'operator*' on
> object 'var2' while it is in the 'consumed' state}}
> > + }
> > +
> > +#if 0
> > + // FIXME: Get this test to pass.
> > + if (var0 || var1 || var2) {
> > + *var0;
> > + *var1;
> > + *var2;
> > +
> > + } else {
> > + *var0; // expected-warning {{invocation of method 'operator*' on
> object 'var0' while it is in the 'consumed' state}}
> > + *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > + *var2; // expected-warning {{invocation of method 'operator*' on
> object 'var2' while it is in the 'consumed' state}}
> > + }
> > +#endif
> > +}
> > +
> > +void testStateChangeInBranch() {
> > + ConsumableClass<int> var;
> > +
> > + // Make var enter the 'unknown' state.
> > + baf1(var);
> > +
> > + if (!var) {
> > + var = ConsumableClass<int>(42);
> > + }
> > +
> > + *var;
> > +}
> > +
> > +void testCallingConventions() {
> > ConsumableClass<int> var(42);
> >
> > baf0(var);
> > @@ -130,7 +240,7 @@
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> > }
> >
> > -void testMoveAsignmentish(void) {
> > +void testMoveAsignmentish() {
> > ConsumableClass<int> var0;
> > ConsumableClass<long> var1(42);
> >
> > @@ -143,26 +253,25 @@
> > *var1; // expected-warning {{invocation of method 'operator*' on
> object 'var1' while it is in the 'consumed' state}}
> > }
> >
> > -void testConditionalMerge(void) {
> > +void testConditionalMerge() {
> > ConsumableClass<int> var;
> >
> > if (var.isValid()) {
> > // Empty
> > }
> >
> > - *var;
> > + *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> >
> > if (var.isValid()) {
> > // Empty
> > -
> > } else {
> > // Empty
> > }
> >
> > - *var;
> > + *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> > }
> >
> > -void testConsumes0(void) {
> > +void testConsumes0() {
> > ConsumableClass<int> var(42);
> >
> > *var;
> > @@ -172,13 +281,13 @@
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> > }
> >
> > -void testConsumes1(void) {
> > +void testConsumes1() {
> > ConsumableClass<int> var(nullptr);
> >
> > *var; // expected-warning {{invocation of method 'operator*' on
> object 'var' while it is in the 'consumed' state}}
> > }
> >
> > -void testConsumes2(void) {
> > +void testConsumes2() {
> > ConsumableClass<int> var(42);
> >
> > var.unconsumedCall();
> > @@ -187,7 +296,19 @@
> > var.unconsumedCall(); // expected-warning {{invocation of method
> 'unconsumedCall' on object 'var' while it is in the 'consumed' state}}
> > }
> >
> > -void testSimpleForLoop(void) {
> > +void testNonsenseState() {
> > + ConsumableClass<int> var(42);
> > +
> > + if (var) {
> > + *var;
> > + } else {
> > + *var;
> > + }
> > +
> > + *var;
> > +}
> > +
> > +void testSimpleForLoop() {
> > ConsumableClass<int> var;
> >
> > for (int i = 0; i < 10; ++i) {
> > @@ -197,7 +318,7 @@
> > *var;
> > }
> >
> > -void testSimpleWhileLoop(void) {
> > +void testSimpleWhileLoop() {
> > int i = 0;
> >
> > ConsumableClass<int> var;
> >
>
> On Fri, Aug 23, 2013 at 8:58 PM, Christian Wailes
> <chriswailes at google.com> wrote:
> >
> > This depends on patch http://llvm-reviews.chandlerc.com/D1500
> >
> > http://llvm-reviews.chandlerc.com/D1501
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130826/d467e536/attachment.html>
More information about the cfe-commits
mailing list