From 81a053aa900269f89f69b194b33fac32db92be25 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Sun, 18 Jul 2010 10:18:41 +0200 Subject: [PATCH] Fixed #1311 (false negative: missing const not found in derived classes) --- lib/checkclass.cpp | 162 +++++++++++++++++++++++++++++++++++++++++++-- lib/checkclass.h | 3 + test/testclass.cpp | 109 ++++++++++++++++++++++++++++++ 3 files changed, 269 insertions(+), 5 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 554e09851..d1b7dc0d2 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1683,11 +1683,123 @@ void CheckClass::thisSubtraction() } //--------------------------------------------------------------------------- +// check if this function is defined virtual in the base classes +bool CheckClass::isVirtual(const std::vector &derivedFrom, const Token *functionToken) const +{ + // check each base class + for (unsigned int i = 0; i < derivedFrom.size(); ++i) + { + std::string className; + + if (derivedFrom[i].find("::") != std::string::npos) + { + /** @todo handle nested base classes and namespaces */ + } + else + className = derivedFrom[i]; + + std::string classPattern = std::string("class|struct ") + className + std::string(" {|:"); + + // find the base class + const Token *classToken = Token::findmatch(_tokenizer->tokens(), classPattern.c_str()); + + // find the function in the base class + if (classToken) + { + std::vector baseList; + const Token * tok = classToken; + + while (tok->str() != "{") + { + // check for base classes + if (Token::Match(tok, ":|, public|protected|private")) + { + // jump to base class name + tok = tok->tokAt(2); + + std::string base; + + // handle nested base classea and namespacess + while (Token::Match(tok, "%var% ::")) + { + base += tok->str(); + base += " :: "; + tok = tok->tokAt(2); + } + + base += tok->str(); + + // save pattern for base class name + baseList.push_back(base); + } + tok = tok->next(); + } + + tok = tok->next(); + + for (; tok; tok = tok->next()) + { + if (tok->str() == "{") + tok = tok->link(); + else if (tok->str() == "}") + break; + else if (Token::Match(tok, "public:|protected:|private:")) + continue; + else if (tok->str() == "(") + tok = tok->link(); + else if (tok->str() == "virtual") + { + // goto the function name + while (tok->next()->str() != "(") + tok = tok->next(); + + // do the function names match? + if (tok->str() == functionToken->str()) + { + const Token *temp1 = tok->previous(); + const Token *temp2 = functionToken->previous(); + bool returnMatch = true; + + // check for matching return parameters + while (temp1->str() != "virtual") + { + if (temp1->str() != temp2->str()) + { + returnMatch = false; + break; + } + + temp1 = temp1->previous(); + temp2 = temp2->previous(); + } + + // check for matching function parameters + if (returnMatch && argsMatch(tok->tokAt(2), functionToken->tokAt(2))) + { + return true; + } + } + } + } + + if (!baseList.empty()) + { + if (isVirtual(baseList, functionToken)) + return true; + } + } + } + + return false; +} + struct NestInfo { std::string className; - const Token * classEnd; + const Token *classStart; + const Token *classEnd; int levelEnd; + std::vector derivedFrom; }; // Can a function be const? @@ -1709,23 +1821,47 @@ void CheckClass::checkConst() if (level == nestInfo.back().levelEnd) nestInfo.pop_back(); } - else if (Token::Match(tok, "class|struct %var% {")) + else if (Token::Match(tok, "class|struct %var% {|:")) { const Token *classTok = tok; + NestInfo info; // get class name.. std::string classname(tok->strAt(1)); + info.className = classname; + info.classStart = tok; - // goto initial {' + // goto initial '{' while (tok && tok->str() != "{") + { + // check for base classes + if (Token::Match(tok, ":|, public|protected|private")) + { + // jump to base class name + tok = tok->tokAt(2); + + std::string derivedFrom; + + // handle derived base classes + while (Token::Match(tok, "%var% ::")) + { + derivedFrom += tok->str(); + derivedFrom += " :: "; + tok = tok->tokAt(2); + } + + derivedFrom += tok->str(); + + // save pattern for base class name + info.derivedFrom.push_back(derivedFrom); + } tok = tok->next(); + } if (!tok) break; const Token *classEnd = tok->link(); - NestInfo info; - info.className = classname; info.classEnd = classEnd; info.levelEnd = level++; nestInfo.push_back(info); @@ -1800,6 +1936,8 @@ void CheckClass::checkConst() if (functionName == classname) continue; + const Token *functionToken = tok2; + // goto the ')' tok2 = tok2->next()->link(); if (!tok2) @@ -1810,6 +1948,13 @@ void CheckClass::checkConst() { const Token *paramEnd = tok2; + // check if base class function is virtual + if (!info.derivedFrom.empty()) + { + if (isVirtual(info.derivedFrom, functionToken)) + continue; + } + // if nothing non-const was found. write error.. if (checkConstFunc(classname, varlist, paramEnd)) { @@ -1821,6 +1966,13 @@ void CheckClass::checkConst() } else if (Token::simpleMatch(tok2, ") ;")) // not inline { + // check if base class function is virtual + if (!info.derivedFrom.empty()) + { + if (isVirtual(info.derivedFrom, functionToken)) + continue; + } + for (int i = nestInfo.size() - 1; i >= 0; i--) { const Token *found = nestInfo[i].classEnd; diff --git a/lib/checkclass.h b/lib/checkclass.h index 38d22761f..89e4e72f8 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -170,6 +170,9 @@ private: bool isMemberVar(const std::string &classname, const Var *varlist, const Token *tok); bool checkConstFunc(const std::string &classname, const Var *varlist, const Token *tok); + /** @brief check if this function is virtual in the base classes */ + bool isVirtual(const std::vector &derivedFrom, const Token *functionToken) const; + /** * @brief Helper function for operatorEqRetRefThis that checks if there are errors * @param tok The "operator" token in a operator=(.. function diff --git a/test/testclass.cpp b/test/testclass.cpp index b1854c17b..7be54272f 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -136,6 +136,7 @@ private: TEST_CASE(constDelete); // delete member variable => not const TEST_CASE(constLPVOID); // a function that returns LPVOID can't be const TEST_CASE(constFunc); // a function that calls const functions can be const + TEST_CASE(constVirtualFunc); } // Check the operator Equal @@ -3676,6 +3677,114 @@ private: "}"); TODO_ASSERT_EQUALS("[test.cpp:7]: (style) The function 'A::GetVecSize' can be const\n", errout.str()); } + + void constVirtualFunc() + { + // base class has no virtual function + checkConst("class A { };\n" + "class B : public A {\n" + " int b;\n" + "public:\n" + " B() : b(0) { }\n" + " int func() { return b; }\n" + "};"); + ASSERT_EQUALS("[test.cpp:6]: (style) The function 'B::func' can be const\n", errout.str()); + + // base class has no virtual function + checkConst("class A {\n" + "public:\n" + " int func();\n" + "};\n" + "class B : public A {\n" + " int b;\n" + "public:\n" + " B() : b(0) { }\n" + " int func() { return b; }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:9]: (style) The function 'B::func' can be const\n", errout.str()); + + // base class has virtual function + checkConst("class A {\n" + "public:\n" + " virtual int func();\n" + "};\n" + "class B : public A {\n" + " int b;\n" + "public:\n" + " B() : b(0) { }\n" + " int func() { return b; }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + // base class has no virtual function + checkConst("class A {\n" + " int a;\n" + "public:\n" + " A() : a(0) { }\n" + " int func() { return a; }\n" + "};\n" + "class B : public A {\n" + " int b;\n" + "public:\n" + " B() : b(0) { }\n" + " int func() { return b; }\n" + "};\n" + "class C : public B {\n" + " int c;\n" + "public:\n" + " C() : c(0) { }\n" + " int func() { return c; }\n" + "};\n"); + ASSERT_EQUALS("[test.cpp:5]: (style) The function 'A::func' can be const\n" + "[test.cpp:11]: (style) The function 'B::func' can be const\n" + "[test.cpp:17]: (style) The function 'C::func' can be const\n", errout.str()); + + // base class has virtual function + checkConst("class A {\n" + " int a;\n" + "public:\n" + " A() : a(0) { }\n" + " virtual int func() { return a; }\n" + "};\n" + "class B : public A {\n" + " int b;\n" + "public:\n" + " B() : b(0) { }\n" + " int func() { return b; }\n" + "};\n" + "class C : public B {\n" + " int c;\n" + "public:\n" + " C() : c(0) { }\n" + " int func() { return c; }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + // ticket #1311 + checkConst("class X {\n" + " int x;\n" + "public:\n" + " X(int x) : x(x) { }\n" + " int getX() { return x; }\n" + "};\n" + "class Y : public X {\n" + " int y;\n" + "public:\n" + " Y(int x, int y) : X(x), y(y) { }\n" + " int getY() { return y; }\n" + "};\n" + "class Z : public Y {\n" + " int z;\n" + "public:\n" + " Z(int x, int y, int z) : Y(x, y), z(z) { }\n" + " int getZ() { return z; }\n" + "};\n" + " }\n" + "};"); + ASSERT_EQUALS("[test.cpp:5]: (style) The function 'X::getX' can be const\n" + "[test.cpp:11]: (style) The function 'Y::getY' can be const\n" + "[test.cpp:17]: (style) The function 'Z::getZ' can be const\n", errout.str()); + } }; REGISTER_TEST(TestClass)