diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 4f31f75a7..aff422593 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -260,6 +260,25 @@ bool astIsContainerOwned(const Token* tok) { return astIsContainer(tok) && !astIsContainerView(tok); } +static const Token* getContainerFunction(const Token* tok) +{ + if (!tok || !tok->valueType() || !tok->valueType()->container) + return nullptr; + const Token* parent = tok->astParent(); + if (Token::Match(parent, ". %name% (") && astIsLHS(tok)) { + return parent->next(); + } + return nullptr; +} + +Library::Container::Action astContainerAction(const Token* tok) +{ + const Token* ftok = getContainerFunction(tok); + if (!ftok) + return Library::Container::Action::NO_ACTION; + return tok->valueType()->container->getAction(ftok->str()); +} + bool astIsRangeBasedForDecl(const Token* tok) { return Token::simpleMatch(tok->astParent(), ":") && Token::simpleMatch(tok->astParent()->astParent(), "("); diff --git a/lib/astutils.h b/lib/astutils.h index d05660789..0374573b8 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -139,6 +139,8 @@ bool astIsContainer(const Token *tok); bool astIsContainerView(const Token* tok); bool astIsContainerOwned(const Token* tok); +Library::Container::Action astContainerAction(const Token* tok); + /** Is given token a range-declaration in a range-based for loop */ bool astIsRangeBasedForDecl(const Token* tok); diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 5bc852df3..156927709 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -607,15 +607,23 @@ struct ForwardTraversal { } else if (condTok->values().front().intvalue == inElse) { return Break(); } - // Handle for loop - Token* stepTok = getStepTokFromEnd(tok); - bool checkThen, checkElse; - std::tie(checkThen, checkElse) = evalCond(condTok); - if (stepTok && !checkElse) { - if (updateRecursive(stepTok) == Progress::Break) - return Break(); - if (updateRecursive(condTok) == Progress::Break) - return Break(); + // Handle loop + if (inLoop) { + Token* stepTok = getStepTokFromEnd(tok); + bool checkThen, checkElse; + std::tie(checkThen, checkElse) = evalCond(condTok); + if (stepTok && !checkElse) { + if (updateRecursive(stepTok) == Progress::Break) + return Break(); + if (updateRecursive(condTok) == Progress::Break) + return Break(); + // Reevaluate condition + std::tie(checkThen, checkElse) = evalCond(condTok); + } + if (!checkElse) { + if (updateLoopExit(end, tok, condTok, nullptr, stepTok) == Progress::Break) + return Break(); + } } analyzer->assume(condTok, !inElse, Analyzer::Assume::Quiet); if (Token::simpleMatch(tok, "} else {")) @@ -852,7 +860,6 @@ struct ForwardTraversal { return nullptr; return getStepTok(end->link()); } - }; Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr& a, const Settings* settings) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index d45209c47..ac9f6c364 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2108,6 +2108,24 @@ static bool evalAssignment(Value& lhsValue, const std::string& assign, const Val return !error; } +static ValueFlow::Value::MoveKind isMoveOrForward(const Token* tok) +{ + if (!tok) + return ValueFlow::Value::MoveKind::NonMovedVariable; + const Token* parent = tok->astParent(); + if (!Token::simpleMatch(parent, "(")) + return ValueFlow::Value::MoveKind::NonMovedVariable; + const Token* ftok = parent->astOperand1(); + if (!ftok) + return ValueFlow::Value::MoveKind::NonMovedVariable; + if (Token::simpleMatch(ftok->astOperand1(), "std :: move")) + return ValueFlow::Value::MoveKind::MovedVariable; + if (Token::simpleMatch(ftok->astOperand1(), "std :: forward")) + return ValueFlow::Value::MoveKind::ForwardedVariable; + // TODO: Check for cast + return ValueFlow::Value::MoveKind::NonMovedVariable; +} + template struct SingleRange { T* x; @@ -2416,6 +2434,19 @@ struct ValueFlowAnalyzer : Analyzer { virtual Action isModified(const Token* tok) const { Action read = Action::Read; + const ValueFlow::Value* value = getValue(tok); + if (value) { + // Moving a moved value wont change the moved value + if (value->isMovedValue() && isMoveOrForward(tok) != ValueFlow::Value::MoveKind::NonMovedVariable) + return read; + // Inserting elements to container wont change the lifetime + if (astIsContainer(tok) && value->isLifetimeValue() && + contains({Library::Container::Action::PUSH, + Library::Container::Action::INSERT, + Library::Container::Action::CHANGE_INTERNAL}, + astContainerAction(tok))) + return read; + } bool inconclusive = false; if (isVariableChangedByFunctionCall(tok, getIndirect(tok), getSettings(), &inconclusive)) return read | Action::Invalid; @@ -2424,7 +2455,6 @@ struct ValueFlowAnalyzer : Analyzer { if (isVariableChanged(tok, getIndirect(tok), getSettings(), isCPP())) { if (Token::Match(tok->astParent(), "*|[|.|++|--")) return read | Action::Invalid; - const ValueFlow::Value* value = getValue(tok); // Check if its assigned to the same value if (value && !value->isImpossible() && Token::simpleMatch(tok->astParent(), "=") && astIsLHS(tok) && astIsIntegral(tok->astParent()->astOperand2(), false)) { diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index ddc3dc1cb..9a465f13f 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -3943,7 +3943,9 @@ private: " if (former_hover != pitem)\n" " dosth();\n" "}"); - ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:4] -> [test.cpp:7]: (error) Using pointer to local variable 'item' that is out of scope.\n", errout.str()); + ASSERT_EQUALS( + "[test.cpp:5] -> [test.cpp:3] -> [test.cpp:4] -> [test.cpp:7]: (error) Using pointer to local variable 'item' that is out of scope.\n", + errout.str()); // #6575 check("void trp_deliver_signal() {\n" diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index e09fdd7ab..18a1f744d 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -867,7 +867,7 @@ private: " i=4;\n" " }\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:6]: (error) Array 'a[5]' accessed at index 5, which is out of bounds.\n", errout.str()); check("void f()\n" "{\n" diff --git a/test/testother.cpp b/test/testother.cpp index 36ffa85c1..7389c3b9b 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -254,6 +254,7 @@ private: TEST_CASE(moveAndAddressOf); TEST_CASE(partiallyMoved); TEST_CASE(moveAndLambda); + TEST_CASE(moveInLoop); TEST_CASE(forwardAndUsed); TEST_CASE(funcArgNamesDifferent); @@ -9837,6 +9838,25 @@ private: ASSERT_EQUALS("", errout.str()); } + void moveInLoop() + { + check("void g(std::string&& s);\n" + "void f() {\n" + " std::string p;\n" + " while(true)\n" + " g(std::move(p));\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Access of moved variable 'p'.\n", errout.str()); + + check("std::list g(std::list&&);\n" + "void f(std::listl) {\n" + " for(int i = 0; i < 10; ++i) {\n" + " for (auto &j : g(std::move(l))) { (void)j; }\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (warning) Access of moved variable 'l'.\n", errout.str()); + } + void forwardAndUsed() { Settings keepTemplates; keepTemplates.checkUnusedTemplates = true;