Fixed #3048, further improvements to const correctness check.

lib/checkclass.cpp

@@ -1158,11 +1158,11 @@ void CheckClass::thisSubtractionError(const Token *tok)
 
 void CheckClass::checkConst()
 {
-    // this is an inconclusive check - see #3048 for instance
+    // This is an inconclusive check. False positives: #3252, #3322, #3360.
     if (!_settings->inconclusive)
         return;
 
-    if (!_settings->isEnabled("style") || _settings->ifcfg)
+    if (!_settings->isEnabled("style"))
         return;
 
     // Don't check C# and JAVA classes
@@ -1221,13 +1221,11 @@ void CheckClass::checkConst()
                             }
                         }
 
-                        if (allupper && previous->str().size() > 2)
+                        if (allupper && s.size() > 2)
                             continue;
                     }
                 }
 
-                const Token *paramEnd = func->arg->link();
-
                 // check if base class function is virtual
                 if (!scope->derivedFrom.empty()) {
                     if (isVirtualFunc(&(*scope), func->tokenDef))
@@ -1235,7 +1233,7 @@ void CheckClass::checkConst()
                 }
 
                 // if nothing non-const was found. write error..
-                if (checkConstFunc(&(*scope), paramEnd)) {
+                if (checkConstFunc(&(*scope), &*func)) {
                     std::string classname = scope->className;
                     const Scope *nest = scope->nestedIn;
                     while (nest && nest->type != Scope::eGlobal) {
@@ -1366,46 +1364,31 @@ bool CheckClass::isConstMemberFunc(const Scope *scope, const Token *tok)
     return false;
 }
 
-bool CheckClass::checkConstFunc(const Scope *scope, const Token *tok)
+bool CheckClass::checkConstFunc(const Scope *scope, const Function *func)
 {
     // if the function doesn't have any assignment nor function call,
     // it can be a const function..
-    unsigned int indentlevel = 0;
+    for (const Token *tok1 = func->start; tok1 && tok1 != func->start->link(); tok1 = tok1->next()) {
-    bool isconst = true;
-    for (const Token *tok1 = tok; tok1; tok1 = tok1->next()) {
-        if (tok1->str() == "{")
-            ++indentlevel;
-        else if (tok1->str() == "}") {
-            if (indentlevel <= 1)
-                break;
-            --indentlevel;
-        }
-
         // assignment.. = += |= ..
-        else if (tok1->isAssignmentOp()) {
+        if (tok1->isAssignmentOp()) {
             if (tok1->next()->str() == "this") {
-                isconst = false;
+                return(false);
-                break;
             } else if (isMemberVar(scope, tok1->previous())) {
-                isconst = false;
+                return(false);
-                break;
             }
         }
 
         // streaming: <<
         else if (tok1->str() == "<<" && isMemberVar(scope, tok1->previous())) {
-            isconst = false;
+            return(false);
-            break;
         } else if (Token::simpleMatch(tok1->previous(), ") <<") &&
                    isMemberVar(scope, tok1->tokAt(-2))) {
-            isconst = false;
+            return(false);
-            break;
         }
 
         // streaming: >>
         else if (tok1->str() == ">>" && isMemberVar(scope, tok1->next())) {
-            isconst = false;
+            return(false);
-            break;
         }
 
         // increment/decrement (member variable?)..
@@ -1414,24 +1397,21 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Token *tok)
             if (Token::Match(tok1->previous(), "%var%") &&
                 tok1->previous()->str() != "return") {
                 if (isMemberVar(scope, tok1->previous())) {
-                    isconst = false;
+                    return(false);
-                    break;
                 }
             }
 
             // var[...]++ and var[...]--
             else if (tok1->previous()->str() == "]") {
                 if (isMemberVar(scope, tok1->previous()->link()->previous())) {
-                    isconst = false;
+                    return(false);
-                    break;
                 }
             }
 
             // ++var and --var
             else if (Token::Match(tok1->next(), "%var%")) {
                 if (isMemberVar(scope, tok1->next())) {
-                    isconst = false;
+                    return(false);
-                    break;
                 }
             }
         }
@@ -1442,37 +1422,49 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Token *tok)
 
             if (var && (var->typeStartToken()->str() == "map" ||
                         Token::simpleMatch(var->typeStartToken(), "std :: map"))) {
-                isconst = false;
+                return(false);
-                break;
             }
         }
 
         // function call..
-        else if (Token::Match(tok1, "%var% (") &&
+        else if (Token::Match(tok1, "%var% (") && !tok1->isStandardType() &&
-                 !(Token::Match(tok1, "return|c_str|if|string|switch|while|catch|for") || tok1->isStandardType())) {
+                 !Token::Match(tok1, "return|c_str|if|string|switch|while|catch|for")) {
             if (!isConstMemberFunc(scope, tok1)) {
-                isconst = false;
+                return(false);
-                break;
+            }
+            // Member variable given as parameter
+            for (const Token* tok2 = tok1->tokAt(2); tok2 && tok2 != tok1->next()->link(); tok2 = tok2->next()) {
+                if (tok2->str() == "(")
+                    tok2 = tok2->link();
+                else if (tok2->isName() && isMemberVar(scope, tok2))
+                    return(false); // TODO: Only bailout if function takes argument as non-const reference
             }
         } else if (Token::Match(tok1, "%var% < %any% > (")) {
-            isconst = false;
+            return(false);
-            break;
+        } else if (Token::Match(tok1, "%var% . size|empty|cend|crend|cbegin|crbegin|max_size|length|count|capacity|rfind|get_allocator|copy ( )") && tok1->varId()) {
-        } else if (Token::Match(tok1, "%var% . size|empty ( )") && tok1->varId()) {
             // assume all std::*::size() and std::*::empty() are const
             const Variable *var = symbolDatabase->getVariableFromVarId(tok1->varId());
 
             if (var && Token::simpleMatch(var->typeStartToken(), "std ::"))
                 tok1 = tok1->tokAt(4);
+            else
+                return(false);
         }
 
         // delete..
         else if (tok1->str() == "delete") {
-            isconst = false;
+            const Token* end = Token::findsimplematch(tok1->next(), ";")->previous();
-            break;
+            while (end->str() == ")")
+                end = end->previous();
+            if (end->str() == "this")
+                return(false);
+            if (end->isName() && isMemberVar(scope, end))
+                return(false);
+
         }
     }
 
-    return isconst;
+    return(true);
 }
 
 // check if this function is defined virtual in the base classes

lib/checkclass.h

@@ -165,7 +165,7 @@ private:
     // checkConst helper functions
     bool isMemberVar(const Scope *scope, const Token *tok);
     bool isConstMemberFunc(const Scope *scope, const Token *tok);
-    bool checkConstFunc(const Scope *scope, const Token *tok);
+    bool checkConstFunc(const Scope *scope, const Function *func);
     /** @brief check if this function is virtual in the base classes */
     bool isVirtualFunc(const Scope *scope, const Token *functionToken) const;
 

test/testclass.cpp

@@ -184,14 +184,15 @@ private:
         TEST_CASE(const49); // ticket #2795
         TEST_CASE(const50); // ticket #2943
         TEST_CASE(const51); // ticket #3040
-        TEST_CASE(const52); // ticket #3049
+        TEST_CASE(const52); // ticket #3048
-        TEST_CASE(const53); // ticket #3052
+        TEST_CASE(const53); // ticket #3049
-        TEST_CASE(const54);
+        TEST_CASE(const54); // ticket #3052
-        TEST_CASE(const55); // ticket #3149
+        TEST_CASE(const55);
+        TEST_CASE(const56); // ticket #3149
         TEST_CASE(assigningPointerToPointerIsNotAConstOperation);
         TEST_CASE(assigningArrayElementIsNotAConstOperation);
         TEST_CASE(constoperator1);  // operator< can often be const
-        TEST_CASE(constoperator2);	// operator<<
+        TEST_CASE(constoperator2);  // operator<<
         TEST_CASE(constoperator3);
         TEST_CASE(constoperator4);
         TEST_CASE(constincdec);     // increment/decrement => non-const
@@ -3553,7 +3554,7 @@ private:
                       "[test.cpp:3]: (warning) Suspicious pointer subtraction\n", errout.str());
     }
 
-    void checkConst(const char code[], const Settings *s = 0) {
+    void checkConst(const char code[], const Settings *s = 0, bool inconclusive = true) {
         // Clear the error log
         errout.str("");
 
@@ -3563,7 +3564,7 @@ private:
             settings = *s;
         else
             settings.addEnabled("style");
-        settings.inconclusive = true;
+        settings.inconclusive = inconclusive;
 
         // Tokenize..
         Tokenizer tokenizer(&settings, this);
@@ -5609,7 +5610,17 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-    void const52() { // ticket 3049
+    void const52() { // ticket 3048
+        checkConst("class foo {\n"
+                   "    void DoSomething(int &a) const { a = 1; }\n"
+                   "    void DoSomethingElse() { DoSomething(bar); }\n"
+                   "private:\n"
+                   "    int bar;\n"
+                   "};");
+        ASSERT_EQUALS("", errout.str());
+    }
+
+    void const53() { // ticket 3049
         checkConst("class A {\n"
                    "  public:\n"
                    "    A() : foo(false) {};\n"
@@ -5625,7 +5636,7 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-    void const53() { // ticket 3052
+    void const54() { // ticket 3052
         checkConst("class Example {\n"
                    "  public:\n"
                    "    void Clear(void) { Example tmp; (*this) = tmp; }\n"
@@ -5633,7 +5644,7 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-    void const54() {
+    void const55() {
         checkConst("class MyObject {\n"
                    "    int tmp;\n"
                    "    MyObject() : tmp(0) {}\n"
@@ -5643,7 +5654,7 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-    void const55() { // ticket #3149
+    void const56() { // ticket #3149
         checkConst("class MyObject {\n"
                    "public:\n"
                    "    void foo(int x) {\n"
@@ -6350,8 +6361,11 @@ private:
         ASSERT_EQUALS("[test.cpp:2]: (style) Technically the member function 'foo::f' can be const.\n", errout.str());
 
         settings.ifcfg = true;
-        checkConst(code, &settings);
+        checkConst(code, &settings, false);
         ASSERT_EQUALS("", errout.str());
+
+        checkConst(code, &settings, true);
+        ASSERT_EQUALS("[test.cpp:2]: (style) Technically the member function 'foo::f' can be const.\n", errout.str());
     }
 
     void constFriend() { // ticket #1921

test/testpathmatch.cpp

@@ -44,6 +44,10 @@ private:
         TEST_CASE(onemasklongerpath1);
         TEST_CASE(onemasklongerpath2);
         TEST_CASE(onemasklongerpath3);
+        TEST_CASE(twomasklongerpath1);
+        TEST_CASE(twomasklongerpath2);
+        TEST_CASE(twomasklongerpath3);
+        TEST_CASE(twomasklongerpath4);
         TEST_CASE(filemask1);
         TEST_CASE(filemaskdifferentcase);
         TEST_CASE(filemask2);