Move some checks for variable changed from constVariable check to isVariableChanged (#4905)

gui/threadresult.cpp

@@ -96,10 +96,9 @@ void ThreadResult::setFiles(const QStringList &files)
 
     // Determine the total size of all of the files to check, so that we can
     // show an accurate progress estimate
-    quint64 sizeOfFiles = 0;
+    quint64 sizeOfFiles = std::accumulate(files.begin(), files.end(), 0, [](quint64 total, const QString& file) {
-    for (const QString& file : files) {
+        return total + QFile(file).size();
-        sizeOfFiles += QFile(file).size();
+    });
-    }
     mMaxProgress = sizeOfFiles;
 }
 

lib/astutils.cpp

@@ -2477,6 +2477,19 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
         }
     }
 
+    // Check addressof
+    if (tok2->astParent() && tok2->astParent()->isUnaryOp("&") &&
+        isVariableChanged(tok2->astParent(), indirect + 1, settings, depth - 1)) {
+        return true;
+    }
+
+    const ValueType* vt = tok->variable() ? tok->variable()->valueType() : tok->valueType();
+    // If its already const then it cant be modified
+    if (vt) {
+        if (vt->constness & (1 << indirect))
+            return false;
+    }
+
     if (cpp && Token::Match(tok2->astParent(), ">>|&") && astIsRHS(tok2) && isLikelyStreamRead(cpp, tok2->astParent()))
         return true;
 
@@ -2484,22 +2497,15 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
         return true;
 
     // Member function call
-    if (tok->variable() && Token::Match(tok2->astParent(), ". %name%") && isFunctionCall(tok2->astParent()->next()) && tok2->astParent()->astOperand1() == tok2) {
+    if (Token::Match(tok2->astParent(), ". %name%") && isFunctionCall(tok2->astParent()->next()) &&
-        const Variable * var = tok->variable();
+        tok2->astParent()->astOperand1() == tok2) {
         // Member function cannot change what `this` points to
         if (indirect == 0 && astIsPointer(tok))
             return false;
-        bool isConst = var && var->isConst();
-        if (!isConst) {
-            const ValueType * valueType = var->valueType();
-            isConst = (valueType && valueType->pointer == 1 && valueType->constness == 1);
-        }
-        if (isConst)
-            return false;
 
         const Token *ftok = tok->tokAt(2);
-        if (astIsContainer(tok) && tok->valueType() && tok->valueType()->container) {
+        if (astIsContainer(tok) && vt && vt->container) {
-            const Library::Container* c = tok->valueType()->container;
+            const Library::Container* c = vt->container;
             const Library::Container::Action action = c->getAction(ftok->str());
             if (contains({Library::Container::Action::INSERT,
                           Library::Container::Action::ERASE,
@@ -2529,8 +2535,8 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
                 return false;
             }
         }
-        if (settings)
+        if (settings && settings->library.isFunctionConst(ftok))
-            return !settings->library.isFunctionConst(ftok);
+            return false;
 
         const Function * fun = ftok->function();
         if (!fun)
@@ -2538,6 +2544,17 @@ bool isVariableChanged(const Token *tok, int indirect, const Settings *settings,
         return !fun->isConst();
     }
 
+    // Member pointer
+    if (Token::Match(tok2->astParent(), ". * ( & %name% ::")) {
+        const Token* ftok = tok2->astParent()->linkAt(2)->previous();
+        // TODO: Check for pointer to member variable
+        if (!ftok->function() || !ftok->function()->isConst())
+            return true;
+    }
+
+    if (Token::simpleMatch(tok2, "[") && astIsContainer(tok) && vt && vt->container && vt->container->stdAssociativeLike)
+        return true;
+
     const Token *ftok = tok2;
     while (ftok && (!Token::Match(ftok, "[({]") || ftok->isCast()))
         ftok = ftok->astParent();
@@ -2675,7 +2692,7 @@ static bool isExpressionChangedAt(const F& getExprTok,
             aliased = isAliasOf(tok, expr);
         if (!aliased)
             return false;
-        if (isVariableChanged(tok, 1, settings, cpp, depth))
+        if (isVariableChanged(tok, indirect + 1, settings, cpp, depth))
             return true;
         // TODO: Try to traverse the lambda function
         if (Token::Match(tok, "%var% ("))

lib/astutils.h

@@ -337,12 +337,12 @@ bool isThisChanged(const Token* start, const Token* end, int indirect, const Set
 const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int exprid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
 Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int exprid, bool globalvar, const Settings *settings, bool cpp, int depth = 20);
 
-bool isExpressionChanged(const Token* expr,
+CPPCHECKLIB bool isExpressionChanged(const Token* expr,
-                         const Token* start,
+                                     const Token* start,
-                         const Token* end,
+                                     const Token* end,
-                         const Settings* settings,
+                                     const Settings* settings,
-                         bool cpp,
+                                     bool cpp,
-                         int depth = 20);
+                                     int depth = 20);
 
 bool isExpressionChangedAt(const Token* expr,
                            const Token* tok,

lib/checkother.cpp

@@ -1489,49 +1489,6 @@ void CheckOther::checkConstVariable()
             if (castToNonConst)
                 continue;
         }
-        // Do not warn if struct data is changed
-        {
-            bool changeStructData = false;
-            for (const Token* tok = var->nameToken(); tok != scope->bodyEnd && tok != nullptr; tok = tok->next()) {
-                if (tok->variable() == var && Token::Match(tok, "%var% .")) {
-                    const Token *parent = tok;
-                    while (Token::simpleMatch(parent->astParent(), ".") && parent == parent->astParent()->astOperand1())
-                        parent = parent->astParent();
-                    if (parent->valueType() &&
-                        parent->valueType()->pointer > 0 &&
-                        parent->valueType()->constness == 0 &&
-                        isVariableChanged(parent, 1, mSettings, mTokenizer->isCPP())) {
-                        changeStructData = true;
-                        break;
-                    }
-                }
-            }
-            if (changeStructData)
-                continue;
-        }
-        // Calling non-const method using non-const reference
-        if (var->isReference()) {
-            bool callNonConstMethod = false;
-            for (const Token* tok = var->nameToken(); tok != scope->bodyEnd && tok != nullptr; tok = tok->next()) {
-                if (tok->variable() == var) {
-                    if (Token::Match(tok, "%var% . * ( & %name% ::")) {
-                        const Token* ftok = tok->linkAt(3)->previous();
-                        if (!ftok->function() || !ftok->function()->isConst())
-                            callNonConstMethod = true;
-                        break;
-                    }
-                    if (var->valueType() && var->valueType()->container && Token::Match(tok, "%var% [")) { // containers whose operator[] is non-const
-                        const auto& notConst = CheckClass::stl_containers_not_const;
-                        if (notConst.find(var->valueType()->container->startPattern) != notConst.end()) {
-                            callNonConstMethod = true;
-                            break;
-                        }
-                    }
-                }
-            }
-            if (callNonConstMethod)
-                continue;
-        }
 
         constVariableError(var, hasFunction ? function : nullptr);
     }

test/fixture.h

@@ -164,12 +164,13 @@ extern std::ostringstream output;
 #define EXPECT_EQ( EXPECTED, ACTUAL ) assertEquals(__FILE__, __LINE__, EXPECTED, ACTUAL)
 #define REGISTER_TEST( CLASSNAME ) namespace { CLASSNAME instance_ ## CLASSNAME; }
 
-#define LOAD_LIB_2( LIB, NAME ) do { \
+#define LOAD_LIB_2(LIB, NAME)                                                                                          \
-        if (((LIB).load(exename.c_str(), NAME).errorcode != Library::ErrorCode::OK)) { \
+    do {                                                                                                               \
-            complainMissingLib(NAME); \
+        if (((LIB).load(exename.c_str(), NAME).errorcode != Library::ErrorCode::OK)) {                                 \
-            return; \
+            complainMissingLib(NAME);                                                                                  \
-        } \
+            abort();                                                                                                   \
-} while (false)
+        }                                                                                                              \
+    } while (false)
 
 #define PLATFORM( P, T ) do { std::string errstr; assertEquals(__FILE__, __LINE__, true, P.set(cppcheck::Platform::toString(T), errstr, {exename}), errstr); } while (false)
 

test/testastutils.cpp

@@ -42,6 +42,7 @@ private:
         TEST_CASE(isSameExpressionTest);
         TEST_CASE(isVariableChangedTest);
         TEST_CASE(isVariableChangedByFunctionCallTest);
+        TEST_CASE(isExpressionChangedTest);
         TEST_CASE(nextAfterAstRightmostLeafTest);
         TEST_CASE(isUsedAsBool);
     }
@@ -227,6 +228,13 @@ private:
         ASSERT_EQUALS(true, isVariableChanged("void f() {\n"
                                               "    int &a = a;\n"
                                               "}\n", "= a", "}"));
+
+        ASSERT_EQUALS(false, isVariableChanged("void f(const A& a) { a.f(); }", "{", "}"));
+        ASSERT_EQUALS(true,
+                      isVariableChanged("void g(int*);\n"
+                                        "void f(int x) { g(&x); }\n",
+                                        "{",
+                                        "}"));
     }
 
 #define isVariableChangedByFunctionCall(code, pattern, inconclusive) isVariableChangedByFunctionCall_(code, pattern, inconclusive, __FILE__, __LINE__)
@@ -258,6 +266,46 @@ private:
         TODO_ASSERT_EQUALS(false, true, inconclusive);
     }
 
+#define isExpressionChanged(code, var, startPattern, endPattern)                                                       \
+    isExpressionChanged_(code, var, startPattern, endPattern, __FILE__, __LINE__)
+    bool isExpressionChanged_(const char code[],
+                              const char var[],
+                              const char startPattern[],
+                              const char endPattern[],
+                              const char* file,
+                              int line)
+    {
+        Settings settings;
+        LOAD_LIB_2(settings.library, "std.cfg");
+        Tokenizer tokenizer(&settings, this);
+        std::istringstream istr(code);
+        ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line);
+        const Token* const start = Token::findsimplematch(tokenizer.tokens(), startPattern, strlen(startPattern));
+        const Token* const end = Token::findsimplematch(start, endPattern, strlen(endPattern));
+        const Token* const expr = Token::findsimplematch(tokenizer.tokens(), var, strlen(var));
+        return (isExpressionChanged)(expr, start, end, &settings, true);
+    }
+
+    void isExpressionChangedTest()
+    {
+        ASSERT_EQUALS(true, isExpressionChanged("void f(std::map<int, int>& m) { m[0]; }", "m [", "{", "}"));
+        ASSERT_EQUALS(false, isExpressionChanged("void f(const A& a) { a.f(); }", "a .", "{", "}"));
+
+        ASSERT_EQUALS(true,
+                      isExpressionChanged("void g(int*);\n"
+                                          "void f(int x) { g(&x); }\n",
+                                          "x",
+                                          ") {",
+                                          "}"));
+
+        ASSERT_EQUALS(true,
+                      isExpressionChanged("struct S { void f(); int i; };\n"
+                                          "void call_f(S& s) { (s.*(&S::f))(); }\n",
+                                          "s .",
+                                          "{ (",
+                                          "}"));
+    }
+
 #define nextAfterAstRightmostLeaf(code, parentPattern, rightPattern) nextAfterAstRightmostLeaf_(code, parentPattern, rightPattern, __FILE__, __LINE__)
     bool nextAfterAstRightmostLeaf_(const char code[], const char parentPattern[], const char rightPattern[], const char* file, int line) {
         Settings settings;