From 346ecdb53afe47dc47d445622c9361c3994334fa Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 23 Feb 2023 11:05:31 -0600 Subject: [PATCH] Improve valueflow after pushing to container (#4803) --- gui/projectfiledialog.cpp | 6 ++--- lib/astutils.cpp | 46 +++++++++++++++++++++++++++++++++++++-- lib/astutils.h | 2 ++ lib/checkstl.cpp | 24 -------------------- lib/preprocessor.cpp | 2 +- lib/valueflow.cpp | 10 ++++++++- test/testcondition.cpp | 18 ++++++++++++++- test/testvalueflow.cpp | 7 ++++++ 8 files changed, 83 insertions(+), 32 deletions(-) diff --git a/gui/projectfiledialog.cpp b/gui/projectfiledialog.cpp index de1e229eb..44cdd67a2 100644 --- a/gui/projectfiledialog.cpp +++ b/gui/projectfiledialog.cpp @@ -587,7 +587,7 @@ QString ProjectFileDialog::getImportProject() const void ProjectFileDialog::addIncludeDir(const QString &dir) { - if (dir.isNull() || dir.isEmpty()) + if (dir.isEmpty()) return; const QString newdir = QDir::toNativeSeparators(dir); @@ -598,7 +598,7 @@ void ProjectFileDialog::addIncludeDir(const QString &dir) void ProjectFileDialog::addCheckPath(const QString &path) { - if (path.isNull() || path.isEmpty()) + if (path.isEmpty()) return; const QString newpath = QDir::toNativeSeparators(path); @@ -609,7 +609,7 @@ void ProjectFileDialog::addCheckPath(const QString &path) void ProjectFileDialog::addExcludePath(const QString &path) { - if (path.isNull() || path.isEmpty()) + if (path.isEmpty()) return; const QString newpath = QDir::toNativeSeparators(path); diff --git a/lib/astutils.cpp b/lib/astutils.cpp index be81142b2..07d052b96 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2863,10 +2863,52 @@ int getArgumentPos(const Variable* var, const Function* f) return std::distance(f->argumentList.cbegin(), arg_it); } +const Token* getIteratorExpression(const Token* tok) +{ + if (!tok) + return nullptr; + if (tok->isUnaryOp("*")) + return nullptr; + if (!tok->isName()) { + const Token* iter1 = getIteratorExpression(tok->astOperand1()); + if (iter1) + return iter1; + if (tok->str() == "(") + return nullptr; + const Token* iter2 = getIteratorExpression(tok->astOperand2()); + if (iter2) + return iter2; + } else if (Token::Match(tok, "begin|cbegin|rbegin|crbegin|end|cend|rend|crend (")) { + if (Token::Match(tok->previous(), ". %name% ( ) !!.")) + return tok->previous()->astOperand1(); + if (!Token::simpleMatch(tok->previous(), ".") && Token::Match(tok, "%name% ( !!)") && + !Token::simpleMatch(tok->linkAt(1), ") .")) + return tok->next()->astOperand2(); + } + return nullptr; +} + bool isIteratorPair(std::vector args) { - return args.size() == 2 && - ((astIsIterator(args[0]) && astIsIterator(args[1])) || (astIsPointer(args[0]) && astIsPointer(args[1]))); + if (args.size() != 2) + return false; + if (astIsPointer(args[0]) && astIsPointer(args[1])) + return true; + // Check if iterator is from same container + const Token* tok1 = nullptr; + const Token* tok2 = nullptr; + if (astIsIterator(args[0]) && astIsIterator(args[1])) { + tok1 = ValueFlow::getLifetimeObjValue(args[0]).tokvalue; + tok2 = ValueFlow::getLifetimeObjValue(args[1]).tokvalue; + if (!tok1 || !tok2) + return true; + } else { + tok1 = getIteratorExpression(args[0]); + tok2 = getIteratorExpression(args[1]); + } + if (tok1 && tok2) + return tok1->exprId() == tok2->exprId(); + return tok1 || tok2; } const Token *findLambdaStartToken(const Token *last) diff --git a/lib/astutils.h b/lib/astutils.h index 8481e4b88..7476e6281 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -375,6 +375,8 @@ std::vector getArguments(const Token *ftok); int getArgumentPos(const Variable* var, const Function* f); +const Token* getIteratorExpression(const Token* tok); + /** * Are the arguments a pair of iterators/pointers? */ diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index b5b441aae..7daa723c9 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -678,30 +678,6 @@ void CheckStl::sameIteratorExpressionError(const Token *tok) reportError(tok, Severity::style, "sameIteratorExpression", "Same iterators expression are used for algorithm.", CWE664, Certainty::normal); } -static const Token * getIteratorExpression(const Token * tok) -{ - if (!tok) - return nullptr; - if (tok->isUnaryOp("*")) - return nullptr; - if (!tok->isName()) { - const Token *iter1 = getIteratorExpression(tok->astOperand1()); - if (iter1) - return iter1; - if (tok->str() == "(") - return nullptr; - const Token *iter2 = getIteratorExpression(tok->astOperand2()); - if (iter2) - return iter2; - } else if (Token::Match(tok, "begin|cbegin|rbegin|crbegin|end|cend|rend|crend (")) { - if (Token::Match(tok->previous(), ". %name% ( ) !!.")) - return tok->previous()->astOperand1(); - if (!Token::simpleMatch(tok->previous(), ".") && Token::Match(tok, "%name% ( !!)") && !Token::simpleMatch(tok->linkAt(1), ") .")) - return tok->next()->astOperand2(); - } - return nullptr; -} - static const Token* getAddressContainer(const Token* tok) { if (Token::simpleMatch(tok, "[") && tok->astOperand1()) diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp index ab368eea4..212cfeeaa 100644 --- a/lib/preprocessor.cpp +++ b/lib/preprocessor.cpp @@ -498,7 +498,7 @@ static void getConfigs(const simplecpp::TokenList &tokens, std::set if (c.find(configs_ifndef.back()) != std::string::npos) ret.insert(c); else if (c.empty()) - ret.insert(configs.empty() ? configs_ifndef.back() : ""); + ret.insert(""); else ret.insert(c + ";" + configs_ifndef.back()); } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index e8c8fdef5..ceed40ad4 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -665,8 +665,11 @@ static void setTokenValue(Token* tok, setTokenValue(parent->astParent(), v, settings); } else if (yields == Library::Container::Yield::EMPTY) { ValueFlow::Value v(value); - v.intvalue = !v.intvalue; v.valueType = ValueFlow::Value::ValueType::INT; + if (value.isImpossible() && value.intvalue == 0) + v.setKnown(); + else + v.intvalue = !v.intvalue; setTokenValue(parent->astParent(), v, settings); } } else if (Token::Match(parent->previous(), "%name% (")) { @@ -8431,6 +8434,11 @@ static void valueFlowContainerSize(TokenList* tokenlist, value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; value.setKnown(); valueFlowForward(tok->linkAt(2), containerTok, value, tokenlist); + } else if (action == Library::Container::Action::PUSH && !isIteratorPair(getArguments(tok->tokAt(2)))) { + ValueFlow::Value value(0); + value.valueType = ValueFlow::Value::ValueType::CONTAINER_SIZE; + value.setImpossible(); + valueFlowForward(tok->linkAt(2), containerTok, value, tokenlist); } } } diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 34a114c0c..f4813ff18 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -2585,7 +2585,7 @@ private: ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str()); check("void f1(const std::string &s) { if(s.size() < 0) if(s.empty()) {}} "); // <- CheckOther reports: checking if unsigned expression is less than zero - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (style) Condition 's.empty()' is always false\n", errout.str()); check("void f1(const std::string &s) { if(s.empty()) if(s.size() > 42) {}}"); ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str()); @@ -4971,6 +4971,22 @@ private: " return a.empty() || (b.empty() && a.empty());\n" "}\n"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Return value 'a.empty()' is always false\n", errout.str()); + + check("struct A {\n" + " struct iterator;\n" + " iterator begin() const;\n" + " iterator end() const;\n" + "};\n" + "A g();\n" + "void f(bool b) {\n" + " std::set s;\n" + " auto v = g();\n" + " s.insert(v.begin(), v.end());\n" + " if(!b && s.size() != 1)\n" + " return;\n" + " if(!s.empty()) {}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void alwaysTrueLoop() diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 393f1ffcc..e532ce28e 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -6378,6 +6378,13 @@ private: " else if (a.empty() == false && b.empty() == false) {}\n" "}\n"; ASSERT("" != isImpossibleContainerSizeValue(tokenValues(code, "a . empty ( ) == false"), 0)); + + code = "bool g(std::vector& v) {\n" + " v.push_back(1);\n" + " int x = v.empty();\n" + " return x;\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXKnown(code, 4U, 0)); } void valueFlowContainerElement()