From af80344ab73dd133ee3e2d6b49ea94514a08c906 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Wed, 18 Apr 2012 18:51:38 +0200 Subject: [PATCH] Refactorizations in checkclass.cpp: - Removed local isVirtual implementation in checkclass.cpp, use Function::isImplicitlyVirtual instead - Don't bailout when we see C++-style casts in checkConst - Don't bailout for this pattern "any << member << any" - Improved/Fixed some test cases (-> #1305) --- lib/checkclass.cpp | 94 +++++++++------------------------------------- lib/checkclass.h | 2 - test/testclass.cpp | 48 ++++++++++++++++------- 3 files changed, 51 insertions(+), 93 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 8887ec5e7..fcdb2d594 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -552,29 +552,21 @@ void CheckClass::privateFunctions() if (!whole) continue; - std::list FuncList; + std::list FuncList; /** @todo embedded class have access to private functions */ if (!scope->getNestedNonFunctions()) { for (std::list::const_iterator func = scope->functionList.begin(); func != scope->functionList.end(); ++func) { // Get private functions.. if (func->type == Function::eFunction && func->access == Private) - FuncList.push_back(func->tokenDef); + FuncList.push_back(&*func); } } // Bailout for overriden virtual functions of base classes if (!scope->derivedFrom.empty()) { - // All base classes seen? - for (unsigned int i = 0; i < scope->derivedFrom.size() && whole; ++i) { - if (!scope->derivedFrom[i].scope || !scope->derivedFrom[i].scope->classStart) - whole = false; - } - if (!whole) - continue; - // Check virtual functions - for (std::list::iterator i = FuncList.begin(); i != FuncList.end();) { - if (isVirtualFunc(&*scope, *i)) + for (std::list::iterator i = FuncList.begin(); i != FuncList.end();) { + if ((*i)->isImplicitlyVirtual(true)) // Give true as default value to be returned if we don't see all base classes FuncList.erase(i++); else ++i; @@ -582,27 +574,28 @@ void CheckClass::privateFunctions() } while (!FuncList.empty()) { + const std::string& funcName = FuncList.front()->tokenDef->str(); // Check that all private functions are used - bool used = checkFunctionUsage(FuncList.front()->str(), &*scope); // Usage in this class + bool used = checkFunctionUsage(funcName, &*scope); // Usage in this class // Check in friend classes for (std::list::const_iterator i = scope->friendList.begin(); !used && i != scope->friendList.end(); ++i) - used = checkFunctionUsage(FuncList.front()->str(), i->scope); + used = checkFunctionUsage(funcName, i->scope); if (!used) { // Final check; check if the function pointer is used somewhere.. - const std::string _pattern("return|throw|(|)|,|= &|" + FuncList.front()->str()); + const std::string _pattern("return|throw|(|)|,|= &|" + funcName); // or if the function address is used somewhere... // eg. sigc::mem_fun(this, &className::classFunction) - const std::string _pattern2("& " + scope->className + " :: " + FuncList.front()->str()); - const std::string methodAsArgument("(|, " + scope->className + " :: " + FuncList.front()->str() + " ,|)"); - const std::string methodAssigned("%var% = &| " + scope->className + " :: " + FuncList.front()->str()); + const std::string _pattern2("& " + scope->className + " :: " + funcName); + const std::string methodAsArgument("(|, " + scope->className + " :: " + funcName + " ,|)"); + const std::string methodAssigned("%var% = &| " + scope->className + " :: " + funcName); if (!Token::findmatch(_tokenizer->tokens(), _pattern.c_str()) && !Token::findmatch(_tokenizer->tokens(), _pattern2.c_str()) && !Token::findmatch(_tokenizer->tokens(), methodAsArgument.c_str()) && !Token::findmatch(_tokenizer->tokens(), methodAssigned.c_str())) { - unusedPrivateFunctionError(FuncList.front(), scope->className, FuncList.front()->str()); + unusedPrivateFunctionError(FuncList.front()->tokenDef, scope->className, funcName); } } @@ -1182,7 +1175,7 @@ void CheckClass::checkConst() // check if base class function is virtual if (!scope->derivedFrom.empty()) { - if (isVirtualFunc(&(*scope), func->tokenDef)) + if (func->isImplicitlyVirtual(true)) continue; } @@ -1333,7 +1326,7 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func) } // streaming: << - else if (tok1->str() == "<<" && isMemberVar(scope, tok1->previous())) { + else if (tok1->str() == "<<" && isMemberVar(scope, tok1->previous()) && tok1->strAt(-2) != "<<") { return(false); } else if (Token::simpleMatch(tok1->previous(), ") <<") && isMemberVar(scope, tok1->tokAt(-2))) { @@ -1382,7 +1375,7 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func) // function call.. else if (Token::Match(tok1, "%var% (") && !tok1->isStandardType() && - !Token::Match(tok1, "return|c_str|if|string|switch|while|catch|for")) { + !Token::Match(tok1, "return|if|string|switch|while|catch|for")) { if (!isConstMemberFunc(scope, tok1)) { return(false); } @@ -1393,9 +1386,9 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func) 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% > (")) { + } else if (Token::Match(tok1, "> (") && (!tok1->link() || !Token::Match(tok1->link()->previous(), "static_cast|const_cast|dynamic_cast|reinterpret_cast"))) { return(false); - } 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|cend|crend|cbegin|crbegin|max_size|length|count|capacity|rfind|get_allocator|copy|c_str ( )") && tok1->varId()) { // assume all std::*::size() and std::*::empty() are const const Variable *var = symbolDatabase->getVariableFromVarId(tok1->varId()); @@ -1421,59 +1414,6 @@ bool CheckClass::checkConstFunc(const Scope *scope, const Function *func) return(true); } -// check if this function is defined virtual in the base classes -bool CheckClass::isVirtualFunc(const Scope *scope, const Token *functionToken) const -{ - // check each base class - for (unsigned int i = 0; i < scope->derivedFrom.size(); ++i) { - // check if base class exists in database - if (scope->derivedFrom[i].scope) { - const Scope *derivedFrom = scope->derivedFrom[i].scope; - - std::list::const_iterator func; - - // check if function defined in base class - for (func = derivedFrom->functionList.begin(); func != derivedFrom->functionList.end(); ++func) { - if (func->isVirtual) { - const Token *tok = func->tokenDef; - - 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 && Function::argsMatch(scope, tok->tokAt(2), functionToken->tokAt(2), std::string(""), 0)) { - return true; - } - } - } - } - - if (!derivedFrom->derivedFrom.empty()) { - if (isVirtualFunc(derivedFrom, functionToken)) - return true; - } - } else { - // unable to find base class so assume it has a virtual function - return true; - } - } - - return false; -} - void CheckClass::checkConstError(const Token *tok, const std::string &classname, const std::string &funcname) { reportInconclusiveError(tok, Severity::style, "functionConst", diff --git a/lib/checkclass.h b/lib/checkclass.h index 21652c163..462003974 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -166,8 +166,6 @@ private: bool isMemberVar(const Scope *scope, const Token *tok); bool isConstMemberFunc(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; // constructors helper function /** @brief Information about a member variable. Used when checking for uninitialized variables */ diff --git a/test/testclass.cpp b/test/testclass.cpp index 256f7d4fe..9b10d37bb 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -3629,15 +3629,23 @@ private: "};\n"); ASSERT_EQUALS("", errout.str()); - // functions with a function call can't be const.. - checkConst("class foo\n" - "{\n" + // functions with a call to a member function can only be const, if that member function is const, too.. (#1305) + checkConst("class foo {\n" "public:\n" " int x;\n" + " void a() { x = 1; }\n" " void b() { a(); }\n" - "};\n"); + "};"); ASSERT_EQUALS("", errout.str()); + checkConst("class Fred {\n" + "public:\n" + " int x;\n" + " int a() const { return x; }\n" + " void b() { a(); }\n" + "};"); + ASSERT_EQUALS("[test.cpp:5]: (style) Technically the member function 'Fred::b' can be const.\n", errout.str()); + // static functions can't be const.. checkConst("class foo\n" "{\n" @@ -3787,11 +3795,12 @@ private: "int Fred::setA() { a |= true; }"); ASSERT_EQUALS("", errout.str()); - // functions with a function call can't be const.. + // functions with a function call to a non-const member can't be const.. (#1305) checkConst("class Fred\n" "{\n" "public:\n" " int x;\n" + " void a() { x = 1; }\n" " void b();\n" "};\n" "void Fred::b() { a(); }"); @@ -4053,6 +4062,18 @@ private: " }\n" "};\n"); ASSERT_EQUALS("", errout.str()); + + checkConst("struct Foo {\n" + " void operator<<(int);\n" + "};\n" + "struct Fred {\n" + " Foo foo;\n" + " void x()\n" + " {\n" + " std::cout << foo << 123;\n" + " }\n" + "};"); + ASSERT_EQUALS("[test.cpp:6]: (style) Technically the member function 'Fred::x' can be const.\n", errout.str()); } void constoperator3() { @@ -4669,15 +4690,6 @@ private: "};\n"); ASSERT_EQUALS("", errout.str()); - // functions with a function call can't be const.. - checkConst("class foo\n" - "{\n" - "public:\n" - " unsigned long long int x;\n" - " void b() { a(); }\n" - "};\n"); - ASSERT_EQUALS("", errout.str()); - // static functions can't be const.. checkConst("class foo\n" "{\n" @@ -4899,6 +4911,14 @@ private: "};\n" ); ASSERT_EQUALS("", errout.str()); + + checkConst("struct DelayBase {\n" + " float swapSpecificDelays(int index1) {\n" + " return static_cast(delays_[index1]);\n" + " }\n" + " float delays_[4];\n" + "};"); + ASSERT_EQUALS("[test.cpp:2]: (style) Technically the member function 'DelayBase::swapSpecificDelays' can be const.\n", errout.str()); } void const27() { // ticket #1882