Fix #13000 FP knownConditionTrueFalse with std::string::append()

danmar · Aug 9, 2024 · f2058b8 · f2058b8
1 parent 9c3c0ed
commit f2058b8
Show file tree

Hide file tree

Showing 9 changed files with 38 additions and 6 deletions.
diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng
@@ -709,6 +709,7 @@
       <value>find-const</value>
       <value>insert</value>
       <value>erase</value>
+      <value>append</value>
       <value>change-content</value>
       <value>change-internal</value>
       <value>change</value>

diff --git a/cfg/qt.cfg b/cfg/qt.cfg
@@ -5208,6 +5208,7 @@
       <function name="reserve" action="change-internal"/>
       <function name="chop" action="change"/>
       <function name="remove" action="change"/>
+      <function name="append" action="append"/>
     </size>
     <access indexOperator="array-like">
       <function name="at" yields="at_index"/>

diff --git a/cfg/std.cfg b/cfg/std.cfg
@@ -8913,7 +8913,7 @@ initializer list (7) string& replace (const_iterator i1, const_iterator i2, init
     <size>
       <function name="push_back" action="push"/>
       <function name="pop_back" action="pop"/>
-      <function name="append" action="push"/>
+      <function name="append" action="append"/>
       <function name="replace" action="change"/>
       <function name="reserve" action="change-internal"/>
       <function name="shrink_to_fit" action="change-internal"/>

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<const Token*> 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,23 @@ 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<const Token*> 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();
+            }
+            default:
+                break;
+            }
         }
         if (d == Direction::Reverse)
             val->intvalue -= n;
@@ -6712,6 +6728,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 +7019,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 +7192,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
@@ -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()