Fixed #1321 (Improve check const-function: function that is not implemented inline can be made const)

lib/checkclass.cpp

@@ -1443,6 +1443,7 @@ void CheckClass::thisSubtraction()
         tok = tok->next();
     }
 }
+//---------------------------------------------------------------------------
 
 const Token * findParameter(const Token *var, const Token *start, const Token * end)
 {
@@ -1459,18 +1460,35 @@ const Token * findParameter(const Token *var, const Token *start, const Token *
     return 0;
 }
 
+struct NestInfo
+{
+    std::string className;
+    const Token * classEnd;
+    int levelEnd;
+};
+
 // Can a function be const?
 void CheckClass::checkConst()
 {
     if (!_settings->_checkCodingStyle)
         return;
 
+    std::vector<NestInfo> nestInfo;
+    int level = 0;
     for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next())
     {
-        if (Token::Match(tok, "class|struct %var% {"))
+        if (tok->str() == "{" && !nestInfo.empty())
+            level++;
+        else if (tok->str() == "}" && !nestInfo.empty())
+        {
+            level--;
+            if (level == nestInfo.back().levelEnd)
+                nestInfo.pop_back();
+        }
+        else if (Token::Match(tok, "class|struct %var% {"))
         {
             // get class name..
-            const std::string classname(tok->strAt(1));
+            std::string classname(tok->strAt(1));
 
             // goto initial {'
             while (tok && tok->str() != "{")
@@ -1478,6 +1496,14 @@ void CheckClass::checkConst()
             if (!tok)
                 break;
 
+            const Token *classEnd = tok->link();
+
+            NestInfo info;
+            info.className = classname;
+            info.classEnd = classEnd;
+            info.levelEnd = level++;
+            nestInfo.push_back(info);
+
             // parse in this class definition to see if there are any simple getter functions
             for (const Token *tok2 = tok->next(); tok2; tok2 = tok2->next())
             {
@@ -1516,6 +1542,8 @@ void CheckClass::checkConst()
 
                     // get function name
                     const std::string functionName((tok2->isName() ? "" : "operator") + tok2->str());
+
+                    // skip constructor
                     if (functionName == classname)
                         continue;
 
@@ -1524,17 +1552,172 @@ void CheckClass::checkConst()
                     if (!tok2)
                         break;
 
+                    // is this a non-const function that is implemented inline?
+                    if (Token::simpleMatch(tok2, ") {"))
+                    {
                         const Token *paramEnd = tok2;
                         const Token *paramStart = tok2->link();
 
-                    // is this a non-const function that is implemented inline?
+                        // if nothing non-const was found. write error..
-                    if (Token::simpleMatch(tok2, ") {"))
+                        if (checkConstFunc(paramStart, paramEnd))
+                        {
+                            for (int i = nestInfo.size() - 2; i >= 0; i--)
+                                classname = std::string(nestInfo[i].className + "::" + classname);
+
+                            checkConstError(tok2, classname, functionName);
+                        }
+                    }
+                    else if (Token::simpleMatch(tok2, ") ;")) // not inline
+                    {
+                        for (int i = nestInfo.size() - 1; i >= 0; i--)
+                        {
+                            const Token *found = nestInfo[i].classEnd;
+                            std::string pattern(functionName + " (");
+                            int level = 0;
+                            for (int j = nestInfo.size() - 1; j >= i; j--, level++)
+                                pattern = std::string(nestInfo[j].className + " :: " + pattern);
+                            while ((found = Token::findmatch(found->next(), pattern.c_str())))
+                            {
+                                const Token *paramStart = found->tokAt(1 + (2 * level));
+                                const Token *paramEnd = paramStart->link();
+                                if (!paramEnd)
+                                    break;
+                                if (paramEnd->next()->str() != "{")
+                                    break;
+
+                                if (sameFunc(level, tok2, paramEnd))
+                                {
+                                    // if nothing non-const was found. write error..
+                                    if (checkConstFunc(paramStart, paramEnd))
+                                    {
+                                        for (int k = nestInfo.size() - 2; k >= 0; k--)
+                                            classname = std::string(nestInfo[k].className + "::" + classname);
+
+                                        checkConstError2(found, tok2, classname, functionName);
+                                    }
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
+bool CheckClass::sameFunc(int nest, const Token *firstEnd, const Token *secondEnd)
+{
+    // check return type (search backwards until previous statement)
+    const Token * firstStart = firstEnd->link()->tokAt(-2);
+    const Token * secondStart = secondEnd->link()->tokAt((-2 * nest) - 2);
+
+    bool firstDone = false;
+    bool secondDone = false;
+
+    while (true)
+    {
+        firstDone = false;
+        secondDone = false;
+
+        if (!firstStart || Token::Match(firstStart, ";|}|{|public:|protected:|private:"))
+            firstDone = true;
+
+        if (!secondStart || Token::Match(secondStart, ";|}|{|public:|protected:|private:"))
+            secondDone = true;
+
+        if (firstDone != secondDone)
+            return false;
+
+        // both done and match
+        if (firstDone)
+            break;
+
+        if (secondStart->str() != firstStart->str())
+            return false;
+
+        firstStart = firstStart->previous();
+        secondStart = secondStart->previous();
+    }
+
+    // check parameter types (names can be different or missing)
+    firstStart = firstEnd->link()->next();
+    secondStart = secondEnd->link()->next();
+
+    while (true)
+    {
+        firstDone = false;
+        secondDone = false;
+        bool again = true;
+
+        while (again)
+        {
+            again = false;
+
+            if (firstStart == firstEnd)
+                firstDone = true;
+
+            if (secondStart == secondEnd)
+                secondDone = true;
+
+            // possible difference in number of parameters
+            if (firstDone != secondDone)
+            {
+                // check for missing names
+                if (firstDone)
+                {
+                    if (secondStart->varId() != 0)
+                        again = true;
+                }
+                else
+                {
+                    if (firstStart->varId() != 0)
+                        again = true;
+                }
+
+                if (!again)
+                    return false;
+            }
+
+            // both done and match
+            if (firstDone && !again)
+                return true;
+
+            if (firstStart->varId() != 0)
+            {
+                // skip variable name
+                firstStart = firstStart->next();
+                again = true;
+            }
+
+            if (secondStart->varId() != 0)
+            {
+                // skip variable name
+                secondStart = secondStart->next();
+                again = true;
+            }
+        }
+
+        if (firstStart->str() != secondStart->str())
+            return false;
+
+        // retry after skipped variable names
+        if (!again)
+        {
+            firstStart = firstStart->next();
+            secondStart = secondStart->next();
+        }
+    }
+
+    return true;
+}
+
+bool CheckClass::checkConstFunc(const Token *paramStart, const Token *paramEnd)
 {
     // if the function doesn't have any assignment nor function call,
     // it can be a const function..
     unsigned int indentlevel = 0;
     bool isconst = true;
-                        for (const Token *tok3 = tok2; tok3; tok3 = tok3->next())
+    for (const Token *tok3 = paramEnd; tok3; tok3 = tok3->next())
     {
         if (tok3->str() == "{")
             ++indentlevel;
@@ -1594,16 +1777,7 @@ void CheckClass::checkConst()
         }
     }
 
-                        // nothing non-const was found. write error..
+    return isconst;
-                        if (isconst)
-                            checkConstError(tok2, classname, functionName);
-                    }
-                }
-            }
-
-        }
-    }
-
 }
 
 void CheckClass::checkConstError(const Token *tok, const std::string &classname, const std::string &funcname)
@@ -1611,7 +1785,13 @@ void CheckClass::checkConstError(const Token *tok, const std::string &classname,
     reportError(tok, Severity::style, "functionConst", "The function '" + classname + "::" + funcname + "' can be const");
 }
 
-
+void CheckClass::checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname)
+{
+    std::list<const Token *> toks;
+    toks.push_back(tok1);
+    toks.push_back(tok2);
+    reportError(toks, Severity::style, "functionConst", "The function '" + classname + "::" + funcname + "' can be const");
+}
 
 void CheckClass::noConstructorError(const Token *tok, const std::string &classname, bool isStruct)
 {
@@ -1662,4 +1842,3 @@ void CheckClass::operatorEqToSelfError(const Token *tok)
 {
     reportError(tok, Severity::possibleStyle, "operatorEqToSelf", "'operator=' should check for assignment to self");
 }
-

lib/checkclass.h

@@ -152,6 +152,9 @@ private:
     // Check constructors for a specified class
     void checkConstructors(const Token *tok1, const std::string &funcname, bool hasPrivateConstructor, bool isStruct);
 
+    bool sameFunc(int nest, const Token *firstEnd, const Token *secondEnd);
+    bool checkConstFunc(const Token *paramStart, const Token *paramEnd);
+
     // Reporting errors..
     void noConstructorError(const Token *tok, const std::string &classname, bool isStruct);
     void uninitVarError(const Token *tok, const std::string &classname, const std::string &varname, bool hasPrivateConstructor);
@@ -166,6 +169,7 @@ private:
     void operatorEqToSelfError(const Token *tok);
 
     void checkConstError(const Token *tok, const std::string &classname, const std::string &funcname);
+    void checkConstError2(const Token *tok1, const Token *tok2, const std::string &classname, const std::string &funcname);
 
     void getErrorMessages()
     {
@@ -181,6 +185,7 @@ private:
         operatorEqRetRefThisError(0);
         operatorEqToSelfError(0);
         checkConstError(0, "class", "function");
+        checkConstError2(0, 0, "class", "function");
     }
 
     std::string name() const

test/testclass.cpp

@@ -87,6 +87,7 @@ private:
         TEST_CASE(const1);
         TEST_CASE(const2);
         TEST_CASE(const3);
+        TEST_CASE(const4);
         TEST_CASE(constoperator);   // operator< can often be const
         TEST_CASE(constincdec);     // increment/decrement => non-const
         TEST_CASE(constReturnReference);
@@ -1770,6 +1771,273 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void const4()
+    {
+        checkConst("class Fred {\n"
+                   "    int a;\n"
+                   "    int getA();\n"
+                   "};\n"
+                   "int Fred::getA() { return a; }");
+        ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (style) The function 'Fred::getA' can be const\n", errout.str());
+
+        checkConst("class Fred {\n"
+                   "    const std::string foo();\n"
+                   "};\n"
+                   "const std::string Fred::foo() { return ""; }");
+        ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:2]: (style) The function 'Fred::foo' can be const\n", errout.str());
+
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    const std::string & foo();\n"
+                   "};\n"
+                   "const std::string & Fred::foo() { return ""; }");
+        ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (style) The function 'Fred::foo' can be const\n", errout.str());
+
+        // constructors can't be const..
+        checkConst("class Fred {\n"
+                   "    int a;\n"
+                   "public:\n"
+                   "    Fred()\n"
+                   "};\n"
+                   "Fred::Fred() { }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment through |=..
+        checkConst("class Fred {\n"
+                   "    int a;\n"
+                   "    int setA();\n"
+                   "};\n"
+                   "int Fred::setA() { a |= true; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // functions with a function call can't be const..
+        checkConst("class foo\n"
+                   "{\n"
+                   "public:\n"
+                   "    int x;\n"
+                   "    void b();\n"
+                   "};\n"
+                   "void Fred::b() { a(); }");
+        ASSERT_EQUALS("", errout.str());
+
+        // static functions can't be const..
+        checkConst("class foo\n"
+                   "{\n"
+                   "public:\n"
+                   "    static unsigned get();\n"
+                   "};\n"
+                   "static unsigned Fred::get() { return 0; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to variable can't be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo()\n"
+                   "};\n"
+                   "void Fred::foo() { s = ""; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to function argument reference can be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string & a);\n"
+                   "};\n"
+                   "void Fred::foo(std::string & a) { a = s; }");
+        ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (style) The function 'Fred::foo' can be const\n", errout.str());
+
+        // assignment to variable can't be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string & a);\n"
+                   "};\n"
+                   "void Fred::foo(std::string & a) { s = a; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to function argument references can be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string & a, std::string & b);\n"
+                   "};\n"
+                   "void Fred::foo(std::string & a, std::string & b) { a = s; b = s; }");
+        ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (style) The function 'Fred::foo' can be const\n", errout.str());
+
+        // assignment to variable, can't be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string & a, std::string & b);\n"
+                   "};\n"
+                   "void Fred::foo(std::string & a, std::string & b) { s = a; s = b; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to variable, can't be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string & a, std::string & b);\n"
+                   "};\n"
+                   "void Fred::foo(std::string & a, std::string & b) { s = a; b = s; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to variable, can't be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string & a, std::string & b);\n"
+                   "};\n"
+                   "void foo(std::string & a, std::string & b) { a = s; s = b; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to function argument pointer can be const
+        checkConst("class Fred {\n"
+                   "    int s;\n"
+                   "    void foo(int * a);\n"
+                   "};\n"
+                   "void Fred::foo(int * a) { *a = s; }");
+        ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (style) The function 'Fred::foo' can be const\n", errout.str());
+
+        // assignment to variable, can't be const
+        checkConst("class Fred {\n"
+                   "    int s;\n"
+                   "    void foo(int * a);\n"
+                   "};\n"
+                   "void Fred::foo(int * a) { s = *a; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to function argument pointers can be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string * a, std::string * b);\n"
+                   "};\n"
+                   "void Fred::foo(std::string * a, std::string * b) { *a = s; *b = s; }");
+        ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:3]: (style) The function 'Fred::foo' can be const\n", errout.str());
+
+        // assignment to variable, can't be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string * a, std::string * b);\n"
+                   "};\n"
+                   "void Fred::foo(std::string * a, std::string * b) { s = *a; s = *b; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to variable, can't be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string * a, std::string * b);\n"
+                   "};\n"
+                   "void Fred::foo(std::string * a, std::string * b) { s = *a; *b = s; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // assignment to variable, can't be const
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo(std::string * a, std::string * b);\n"
+                   "};\n"
+                   "void Fred::foo(std::string * a, std::string * b) { *a = s; s = b; }");
+        ASSERT_EQUALS("", errout.str());
+
+        // check functions with same name
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo();\n"
+                   "    void foo(std::string & a);\n"
+                   "    void foo(const std::string & a);\n"
+                   "};\n"
+                   "void Fred::foo() { }"
+                   "void Fred::foo(std::string & a) { a = s; }"
+                   "void Fred::foo(const std::string & a) { s = a; }");
+        ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:3]: (style) The function 'Fred::foo' can be const\n"
+                      "[test.cpp:7] -> [test.cpp:4]: (style) The function 'Fred::foo' can be const\n", errout.str());
+
+        // check functions with different or missing paramater names
+        checkConst("class Fred {\n"
+                   "    std::string s;\n"
+                   "    void foo1(int, int);\n"
+                   "    void foo2(int a, int b);\n"
+                   "    void foo3(int, int b);\n"
+                   "    void foo4(int a, int);\n"
+                   "    void foo5(int a, int b);\n"
+                   "};\n"
+                   "void Fred::foo1(int a, int b) { }\n"
+                   "void Fred::foo2(int c, int d) { }\n"
+                   "void Fred::foo3(int a, int b) { }\n"
+                   "void Fred::foo4(int a, int b) { }\n"
+                   "void Fred::foo5(int, int) { }");
+        ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:3]: (style) The function 'Fred::foo1' can be const\n"
+                      "[test.cpp:10] -> [test.cpp:4]: (style) The function 'Fred::foo2' can be const\n"
+                      "[test.cpp:11] -> [test.cpp:5]: (style) The function 'Fred::foo3' can be const\n"
+                      "[test.cpp:12] -> [test.cpp:6]: (style) The function 'Fred::foo4' can be const\n"
+                      "[test.cpp:13] -> [test.cpp:7]: (style) The function 'Fred::foo5' can be const\n", errout.str());
+
+        // check nested classes
+        checkConst("class Fred {\n"
+                   "    class A {\n"
+                   "        int a;\n"
+                   "        int getA() { return a; }\n"
+                   "    };\n"
+                   "};");
+        ASSERT_EQUALS("[test.cpp:4]: (style) The function 'Fred::A::getA' can be const\n", errout.str());
+
+        checkConst("class Fred {\n"
+                   "    class A {\n"
+                   "        int a;\n"
+                   "        int getA();\n"
+                   "    };\n"
+                   "    int A::getA() { return a; }\n"
+                   "};");
+        ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:4]: (style) The function 'Fred::A::getA' can be const\n", errout.str());
+
+        checkConst("class Fred {\n"
+                   "    class A {\n"
+                   "        int a;\n"
+                   "        int getA();\n"
+                   "    };\n"
+                   "};\n"
+                   "int Fred::A::getA() { return a; }");
+        ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:4]: (style) The function 'Fred::A::getA' can be const\n", errout.str());
+
+        // check deeply nested classes
+        checkConst("class Fred {\n"
+                   "    class B {\n"
+                   "        int b;\n"
+                   "        int getB() { return b; }\n"
+                   "        class A {\n"
+                   "            int a;\n"
+                   "            int getA() { return a; }\n"
+                   "        };\n"
+                   "    };\n"
+                   "};");
+        ASSERT_EQUALS("[test.cpp:4]: (style) The function 'Fred::B::getB' can be const\n"
+                      "[test.cpp:7]: (style) The function 'Fred::B::A::getA' can be const\n", errout.str());
+
+        checkConst("class Fred {\n"
+                   "    class B {\n"
+                   "        int b;\n"
+                   "        int getB();\n"
+                   "        class A {\n"
+                   "            int a;\n"
+                   "            int getA();\n"
+                   "        };\n"
+                   "        int A::getA() { return a; }\n"
+                   "    };\n"
+                   "    int B::getB() { return b; }\n"
+                   "};");
+        ASSERT_EQUALS("[test.cpp:11] -> [test.cpp:4]: (style) The function 'Fred::B::getB' can be const\n"
+                      "[test.cpp:9] -> [test.cpp:7]: (style) The function 'Fred::B::A::getA' can be const\n", errout.str());
+
+        checkConst("class Fred {\n"
+                   "    class B {\n"
+                   "        int b;\n"
+                   "        int getB();\n"
+                   "        class A {\n"
+                   "            int a;\n"
+                   "            int getA();\n"
+                   "        };\n"
+                   "    };\n"
+                   "};\n"
+                   "int Fred::B::A::getA() { return a; }\n"
+                   "int Fred::B::getB() { return b; }\n");
+        ASSERT_EQUALS("[test.cpp:12] -> [test.cpp:4]: (style) The function 'Fred::B::getB' can be const\n"
+                      "[test.cpp:11] -> [test.cpp:7]: (style) The function 'Fred::B::A::getA' can be const\n", errout.str());
+    }
+
     // operator< can often be const
     void constoperator()
     {