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