From 3dd3480a94b8b3c88a43ba8388d909ea78f7c05f Mon Sep 17 00:00:00 2001
From: chrchr-github <78114321+chrchr-github@users.noreply.github.com>
Date: Sat, 10 Aug 2024 01:18:07 +0200
Subject: [PATCH] Fix #13000 FP knownConditionTrueFalse with
std::string::append() (#6676)
---
cfg/cppcheck-cfg.rng | 1 +
cfg/qt.cfg | 1 +
cfg/std.cfg | 2 +-
lib/astutils.cpp | 1 +
lib/checkstl.cpp | 1 +
lib/library.cpp | 2 ++
lib/library.h | 1 +
lib/valueflow.cpp | 29 ++++++++++++++++++++++++-----
test/testvalueflow.cpp | 7 +++++++
9 files changed, 39 insertions(+), 6 deletions(-)
diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng
index 1651a740c80..36df004cb2f 100644
--- a/cfg/cppcheck-cfg.rng
+++ b/cfg/cppcheck-cfg.rng
@@ -709,6 +709,7 @@
find-const
insert
erase
+ append
change-content
change-internal
change
diff --git a/cfg/qt.cfg b/cfg/qt.cfg
index 2ef1093c17f..9f6540a310c 100644
--- a/cfg/qt.cfg
+++ b/cfg/qt.cfg
@@ -5208,6 +5208,7 @@
+
diff --git a/cfg/std.cfg b/cfg/std.cfg
index 50d9d197c9c..5a8e75f895f 100644
--- a/cfg/std.cfg
+++ b/cfg/std.cfg
@@ -8913,7 +8913,7 @@ initializer list (7) string& replace (const_iterator i1, const_iterator i2, init
-
+
diff --git a/lib/astutils.cpp b/lib/astutils.cpp
index c80cebeee48..77830788b40 100644
--- a/lib/astutils.cpp
+++ b/lib/astutils.cpp
@@ -2670,6 +2670,7 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings &settings,
const Library::Container::Action action = c->getAction(ftok->str());
if (contains({Library::Container::Action::INSERT,
Library::Container::Action::ERASE,
+ Library::Container::Action::APPEND,
Library::Container::Action::CHANGE,
Library::Container::Action::CHANGE_CONTENT,
Library::Container::Action::CHANGE_INTERNAL,
diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp
index e41443b4880..07fb63ac8cb 100644
--- a/lib/checkstl.cpp
+++ b/lib/checkstl.cpp
@@ -73,6 +73,7 @@ static bool containerAppendsElement(const Library::Container* container, const T
if (Token::Match(parent, ". %name% (")) {
const Library::Container::Action action = container->getAction(parent->strAt(1));
if (contains({Library::Container::Action::INSERT,
+ Library::Container::Action::APPEND,
Library::Container::Action::CHANGE,
Library::Container::Action::CHANGE_INTERNAL,
Library::Container::Action::PUSH,
diff --git a/lib/library.cpp b/lib/library.cpp
index 38cead56997..17ca05ee6cf 100644
--- a/lib/library.cpp
+++ b/lib/library.cpp
@@ -297,6 +297,8 @@ Library::Container::Action Library::Container::actionFrom(const std::string& act
return Container::Action::INSERT;
if (actionName == "erase")
return Container::Action::ERASE;
+ if (actionName == "append")
+ return Container::Action::APPEND;
if (actionName == "change-content")
return Container::Action::CHANGE_CONTENT;
if (actionName == "change-internal")
diff --git a/lib/library.h b/lib/library.h
index 7adc83cd6da..a0742472e36 100644
--- a/lib/library.h
+++ b/lib/library.h
@@ -193,6 +193,7 @@ class CPPCHECKLIB Library {
FIND_CONST,
INSERT,
ERASE,
+ APPEND,
CHANGE_CONTENT,
CHANGE,
CHANGE_INTERNAL,
diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
index fac53ecb3f2..58a20689f13 100644
--- a/lib/valueflow.cpp
+++ b/lib/valueflow.cpp
@@ -1044,6 +1044,7 @@ struct ValueFlowAnalyzer : Analyzer {
if (astIsContainer(tok) && value->isLifetimeValue() &&
contains({Library::Container::Action::PUSH,
Library::Container::Action::INSERT,
+ Library::Container::Action::APPEND,
Library::Container::Action::CHANGE_INTERNAL},
astContainerAction(tok)))
return read;
@@ -6492,6 +6493,8 @@ static bool isContainerSizeChangedByFunction(const Token* tok,
return (isChanged || inconclusive);
}
+static MathLib::bigint valueFlowGetStrLength(const Token* tok);
+
struct ContainerExpressionAnalyzer : ExpressionAnalyzer {
ContainerExpressionAnalyzer(const Token* expr, ValueFlow::Value val, const Settings& s)
: ExpressionAnalyzer(expr, std::move(val), s)
@@ -6527,9 +6530,9 @@ struct ContainerExpressionAnalyzer : ExpressionAnalyzer {
}
} else if (astIsLHS(tok) && Token::Match(tok->astParent(), ". %name% (")) {
const Library::Container::Action action = container->getAction(tok->astParent()->strAt(1));
- if (action == Library::Container::Action::PUSH || action == Library::Container::Action::POP) {
+ if (action == Library::Container::Action::PUSH || action == Library::Container::Action::POP || action == Library::Container::Action::APPEND) { // TODO: handle more actions?
std::vector args = getArguments(tok->tokAt(3));
- if (args.size() < 2)
+ if (args.size() < 2 || action == Library::Container::Action::APPEND)
return Action::Read | Action::Write | Action::Incremental;
}
}
@@ -6563,10 +6566,24 @@ struct ContainerExpressionAnalyzer : ExpressionAnalyzer {
}
} else if (astIsLHS(tok) && Token::Match(tok->astParent(), ". %name% (")) {
const Library::Container::Action action = container->getAction(tok->astParent()->strAt(1));
- if (action == Library::Container::Action::PUSH)
+ switch (action) {
+ case Library::Container::Action::PUSH:
n = 1;
- if (action == Library::Container::Action::POP)
+ break;
+ case Library::Container::Action::POP:
n = -1;
+ break;
+ case Library::Container::Action::APPEND: {
+ std::vector args = getArguments(tok->astParent()->tokAt(2));
+ if (args.size() == 1) // TODO: handle overloads
+ n = valueFlowGetStrLength(tok->astParent()->tokAt(3));
+ if (n == 0) // TODO: handle known empty append
+ val->setPossible();
+ break;
+ }
+ default:
+ break;
+ }
}
if (d == Direction::Reverse)
val->intvalue -= n;
@@ -6712,6 +6729,7 @@ bool ValueFlow::isContainerSizeChanged(const Token* tok, int indirect, const Set
case Library::Container::Action::CHANGE:
case Library::Container::Action::INSERT:
case Library::Container::Action::ERASE:
+ case Library::Container::Action::APPEND:
return true;
case Library::Container::Action::NO_ACTION:
// Is this an unknown member function call?
@@ -7002,7 +7020,7 @@ static const Scope* getFunctionScope(const Scope* scope) {
return scope;
}
-static MathLib::bigint valueFlowGetStrLength(const Token* tok)
+MathLib::bigint valueFlowGetStrLength(const Token* tok)
{
if (tok->tokType() == Token::eString)
return Token::getStrLength(tok);
@@ -7175,6 +7193,7 @@ static void valueFlowContainerSize(const TokenList& tokenlist,
value.setImpossible();
valueFlowForward(tok->linkAt(2), containerTok, std::move(value), tokenlist, errorLogger, settings);
}
+ // TODO: handle more actions?
} else if (tok->str() == "+=" && astIsContainer(tok->astOperand1())) {
const Token* containerTok = tok->astOperand1();
diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp
index 85a38b7bd33..a67f7f2b270 100644
--- a/test/testvalueflow.cpp
+++ b/test/testvalueflow.cpp
@@ -6985,6 +6985,13 @@ class TestValueFlow : public TestFixture {
" if (c.empty()) {}\n"
"}";
ASSERT_EQUALS("", isImpossibleContainerSizeValue(tokenValues(code, "c ."), 2));
+
+ code = "void f(const std::string& a) {\n"
+ " std::string s;\n"
+ " s.append(a);\n"
+ " if (s.empty()) {}\n"
+ "}";
+ ASSERT_EQUALS("", isPossibleContainerSizeValue(tokenValues(code, "s . empty"), 0));
}
void valueFlowContainerElement()