From 7c283d132158754b7ac0d45c204c4d55e6e0b488 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Fri, 5 Mar 2010 17:06:25 +0100 Subject: [PATCH] Fixed #1321 (Improve check const-function: function that is not implemented inline can be made const) --- lib/checkclass.cpp | 305 +++++++++++++++++++++++++++++++++++---------- lib/checkclass.h | 5 + test/testclass.cpp | 268 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 515 insertions(+), 63 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index aeb67a4e4..a17b2faf3 100644 --- a/lib/checkclass.cpp +++ b/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; + 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,86 +1552,232 @@ void CheckClass::checkConst() if (!tok2) break; - const Token *paramEnd = tok2; - const Token *paramStart = tok2->link(); - // is this a non-const function that is implemented inline? if (Token::simpleMatch(tok2, ") {")) { - // 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()) + const Token *paramEnd = tok2; + const Token *paramStart = tok2->link(); + + // if nothing non-const was found. write error.. + if (checkConstFunc(paramStart, paramEnd)) { - if (tok3->str() == "{") - ++indentlevel; - else if (tok3->str() == "}") + 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()))) { - if (indentlevel <= 1) + const Token *paramStart = found->tokAt(1 + (2 * level)); + const Token *paramEnd = paramStart->link(); + if (!paramEnd) + break; + if (paramEnd->next()->str() != "{") break; - --indentlevel; - } - // assignment.. = += |= .. - else if (tok3->str() == "=" || - (tok3->str().find("=") == 1 && - tok3->str().find_first_of("<>") == std::string::npos)) - { - if (tok3->str() == "=") // assignment.. = + if (sameFunc(level, tok2, paramEnd)) { - const Token * param = findParameter(tok3->previous(), paramStart, paramEnd); - if (param) + // if nothing non-const was found. write error.. + if (checkConstFunc(paramStart, paramEnd)) { - // assignment to function argument reference can be const - // f(type & x) { x = something; } - if (param->tokAt(-1)->str() == "&") - continue; + for (int k = nestInfo.size() - 2; k >= 0; k--) + classname = std::string(nestInfo[k].className + "::" + classname); - // assignment to function argument pointer can be const - // f(type * x) { *x = something; } - else if ((param->tokAt(-1)->str() == "*") && - (tok3->tokAt(-2)->str() == "*")) - continue; + checkConstError2(found, tok2, classname, functionName); } } - - isconst = false; - break; - } - - // increment/decrement (member variable?).. - else if (Token::Match(tok3, "++|--")) - { - isconst = false; - break; - } - - // function call.. - else if (tok3->str() != "return" && Token::Match(tok3, "%var% (")) - { - isconst = false; - break; - } - - // delete.. - else if (tok3->str() == "delete") - { - isconst = false; - break; } } - - // nothing non-const was found. write error.. - if (isconst) - checkConstError(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 = paramEnd; tok3; tok3 = tok3->next()) + { + if (tok3->str() == "{") + ++indentlevel; + else if (tok3->str() == "}") + { + if (indentlevel <= 1) + break; + --indentlevel; + } + + // assignment.. = += |= .. + else if (tok3->str() == "=" || + (tok3->str().find("=") == 1 && + tok3->str().find_first_of("<>") == std::string::npos)) + { + if (tok3->str() == "=") // assignment.. = + { + const Token * param = findParameter(tok3->previous(), paramStart, paramEnd); + if (param) + { + // assignment to function argument reference can be const + // f(type & x) { x = something; } + if (param->tokAt(-1)->str() == "&") + continue; + + // assignment to function argument pointer can be const + // f(type * x) { *x = something; } + else if ((param->tokAt(-1)->str() == "*") && + (tok3->tokAt(-2)->str() == "*")) + continue; + } + } + + isconst = false; + break; + } + + // increment/decrement (member variable?).. + else if (Token::Match(tok3, "++|--")) + { + isconst = false; + break; + } + + // function call.. + else if (tok3->str() != "return" && Token::Match(tok3, "%var% (")) + { + isconst = false; + break; + } + + // delete.. + else if (tok3->str() == "delete") + { + isconst = false; + break; + } + } + + return isconst; } 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 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"); } - diff --git a/lib/checkclass.h b/lib/checkclass.h index 6fdbd0bf8..c5c4f7102 100644 --- a/lib/checkclass.h +++ b/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 diff --git a/test/testclass.cpp b/test/testclass.cpp index 5a46e576c..24f0f4795 100644 --- a/test/testclass.cpp +++ b/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() {