[PATCH] Mostly correct conditional handling for Consumed analysis
David Blaikie
dblaikie at gmail.com
Mon Aug 26 11:08:15 PDT 2013
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).
[oops, made this reply, rather than reply-all]
We do have a macro for that (LLVM_EXPLICIT) that I've added to most
operator books in clang and llvm already. That being said, even with
this protection, it's prudent to consider whether op book is the
appropriate tool. Chris - look for existing uses of LLVM_EXPLICIT to
see how to use it, if you consider boolean testability of your type to
be entirely appropriate. Otherwise consider using a free function.
(the test here is simply "is it obvious to the reader what the
true/false property of objects of this type are" - if the test isn't
obvious, giving it a name is a good idea)
(And I share your concerns about the memory management, but as it's an
existing issue, probably best not to gate this patch on addressing
that)
- David
>
> ~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?
>
>>
>> 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.
>
>>
>> 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
>
>> + : 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.
>
>> + 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.
>
>>
>> -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
>
>>
>> - 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
More information about the cfe-commits
mailing list