From 7e54f989f96666c72eb4bb04e7e7c7466969e73f Mon Sep 17 00:00:00 2001 From: shaneasd Date: Thu, 4 Jul 2019 18:32:32 +0800 Subject: [PATCH] Update symbol database such that the override keyword implies that the function is also virtual (#1907) * Update symbol database such that the override keyword implies that the function is also virtual * Add test case for implicit override * change isVirtual to hasVirtualSpecifier * fix method documentation for getVirtualFunctionCalls and getFirstVirtualFunctionCallStack * Fix isImplicitlyVirtual to consider the override keyword and document logic * Fix getFirstVirtualFunctionCallStack and getVirtualFunctionCalls to use isImplicitlyVirtual instead of isVirtual so new test case passes --- lib/checkclass.cpp | 14 +++++++------- lib/checkclass.h | 4 ++-- lib/checkother.cpp | 2 +- lib/symboldatabase.cpp | 20 +++++++++++--------- lib/symboldatabase.h | 10 +++++----- lib/valueflow.cpp | 2 +- test/testclass.cpp | 17 +++++++++++++++++ test/testsymboldatabase.cpp | 4 ++-- 8 files changed, 46 insertions(+), 27 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 4e2060984..5cf2da01a 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -1228,7 +1228,7 @@ void CheckClass::checkMemsetType(const Scope *start, const Token *tok, const Sco // Warn if type is a class that contains any virtual functions for (const Function &func : type->functionList) { - if (func.isVirtual()) { + if (func.hasVirtualSpecifier()) { if (allocation) mallocOnClassError(tok, tok->str(), type->classDef, "virtual function"); else @@ -1651,9 +1651,9 @@ void CheckClass::virtualDestructor() if (scope->definedType->derivedFrom.empty()) { if (printInconclusive) { const Function *destructor = scope->getDestructor(); - if (destructor && !destructor->isVirtual()) { + if (destructor && !destructor->hasVirtualSpecifier()) { for (const Function &func : scope->functionList) { - if (func.isVirtual()) { + if (func.hasVirtualSpecifier()) { inconclusiveErrors.push_back(destructor); break; } @@ -1736,7 +1736,7 @@ void CheckClass::virtualDestructor() if (derivedFrom->derivedFrom.empty()) { virtualDestructorError(derivedFrom->classDef, derivedFrom->name(), derivedClass->str(), false); } - } else if (!baseDestructor->isVirtual()) { + } else if (!baseDestructor->hasVirtualSpecifier()) { // TODO: This is just a temporary fix, better solution is needed. // Skip situations where base class has base classes of its own, because // some of the base classes might have virtual destructor. @@ -1827,7 +1827,7 @@ void CheckClass::checkConst() if (func.type != Function::eFunction || !func.hasBody()) continue; // don't warn for friend/static/virtual functions - if (func.isFriend() || func.isStatic() || func.isVirtual()) + if (func.isFriend() || func.isStatic() || func.hasVirtualSpecifier()) continue; // get last token of return type const Token *previous = func.tokenDef->previous(); @@ -2369,7 +2369,7 @@ const std::list & CheckClass::getVirtualFunctionCalls(const Funct continue; } - if (callFunction->isVirtual()) { + if (callFunction->isImplicitlyVirtual()) { if (!callFunction->isPure() && Token::simpleMatch(tok->previous(), "::")) continue; virtualFunctionCalls.push_back(tok); @@ -2389,7 +2389,7 @@ void CheckClass::getFirstVirtualFunctionCallStack( std::list & pureFuncStack) { const Function *callFunction = callToken->function(); - if (callFunction->isVirtual() && (!callFunction->isPure() || !callFunction->hasBody())) { + if (callFunction->isImplicitlyVirtual() && (!callFunction->isPure() || !callFunction->hasBody())) { pureFuncStack.push_back(callFunction->tokenDef); return; } diff --git a/lib/checkclass.h b/lib/checkclass.h index dae23cfdf..f381d10c6 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -317,7 +317,7 @@ private: void initializeVarList(const Function &func, std::list &callstack, const Scope *scope, std::vector &usage); /** - * @brief gives a list of tokens where pure virtual functions are called directly or indirectly + * @brief gives a list of tokens where virtual functions are called directly or indirectly * @param function function to be checked * @param virtualFunctionCallsMap map of results for already checked functions * @return list of tokens where pure virtual functions are called @@ -327,7 +327,7 @@ private: std::map > & virtualFunctionCallsMap); /** - * @brief looks for the first pure virtual function call stack + * @brief looks for the first virtual function call stack * @param virtualFunctionCallsMap map of results obtained from getVirtualFunctionCalls * @param callToken token where pure virtual function is called directly or indirectly * @param[in,out] pureFuncStack list to append the stack diff --git a/lib/checkother.cpp b/lib/checkother.cpp index d54fe0dd9..0ad20bf0f 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1239,7 +1239,7 @@ void CheckOther::checkPassByReference() } // Check if variable could be const - if (!var->scope() || var->scope()->function->isVirtual()) + if (!var->scope() || var->scope()->function->hasVirtualSpecifier()) continue; if (canBeConst(var)) { diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 4924fd4e9..9f6e49db6 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1757,7 +1757,7 @@ Function::Function(const Tokenizer *mTokenizer, const Token *tok, const Scope *s // virtual function else if (tok1->str() == "virtual") { - isVirtual(true); + hasVirtualSpecifier(true); } // static function @@ -2695,7 +2695,7 @@ void SymbolDatabase::printOut(const char *title) const std::cout << " hasBody: " << func->hasBody() << std::endl; std::cout << " isInline: " << func->isInline() << std::endl; std::cout << " isConst: " << func->isConst() << std::endl; - std::cout << " isVirtual: " << func->isVirtual() << std::endl; + std::cout << " hasVirtualSpecifier: " << func->hasVirtualSpecifier() << std::endl; std::cout << " isPure: " << func->isPure() << std::endl; std::cout << " isStatic: " << func->isStatic() << std::endl; std::cout << " isStaticLocal: " << func->isStaticLocal() << std::endl; @@ -2951,8 +2951,8 @@ void SymbolDatabase::printXml(std::ostream &out) const function->type == Function::eFunction ? "Function" : "Unknown") << '\"'; if (function->nestedIn->definedType) { - if (function->isVirtual()) - out << " isVirtual=\"true\""; + if (function->hasVirtualSpecifier()) + out << " hasVirtualSpecifier=\"true\""; else if (function->isImplicitlyVirtual()) out << " isImplicitlyVirtual=\"true\""; } @@ -3144,14 +3144,16 @@ void Function::addArguments(const SymbolDatabase *symbolDatabase, const Scope *s bool Function::isImplicitlyVirtual(bool defaultVal) const { - if (isVirtual()) + if (hasVirtualSpecifier()) //If it has the virtual specifier it's definitely virtual + return true; + if (hasOverrideSpecifier()) //If it has the override specifier then it's either virtual or not going to compile return true; bool foundAllBaseClasses = true; - if (getOverriddenFunction(&foundAllBaseClasses)) + if (getOverriddenFunction(&foundAllBaseClasses)) //If it overrides a base class's method then it's virtual return true; - if (foundAllBaseClasses) + if (foundAllBaseClasses) //If we've seen all the base classes and none of the above were true then it must not be virtual return false; - return defaultVal; + return defaultVal; //If we can't see all the bases classes then we can't say conclusively } const Function *Function::getOverriddenFunction(bool *foundAllBaseClasses) const @@ -3180,7 +3182,7 @@ const Function * Function::getOverriddenFunctionRecursive(const ::Type* baseType // check if function defined in base class for (std::multimap::const_iterator it = parent->functionMap.find(tokenDef->str()); it != parent->functionMap.end() && it->first == tokenDef->str(); ++it) { const Function * func = it->second; - if (func->isVirtual()) { // Base is virtual and of same name + if (func->hasVirtualSpecifier()) { // Base is virtual and of same name const Token *temp1 = func->tokenDef->previous(); const Token *temp2 = tokenDef->previous(); bool match = true; diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index b4941afc4..d1f272bae 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -665,7 +665,7 @@ class CPPCHECKLIB Function { fHasBody = (1 << 0), ///< @brief has implementation fIsInline = (1 << 1), ///< @brief implementation in class definition fIsConst = (1 << 2), ///< @brief is const - fIsVirtual = (1 << 3), ///< @brief is virtual + fHasVirtualSpecifier = (1 << 3), ///< @brief does declaration contain 'virtual' specifier fIsPure = (1 << 4), ///< @brief is pure virtual fIsStatic = (1 << 5), ///< @brief is static fIsStaticLocal = (1 << 6), ///< @brief is static local @@ -771,8 +771,8 @@ public: bool isConst() const { return getFlag(fIsConst); } - bool isVirtual() const { - return getFlag(fIsVirtual); + bool hasVirtualSpecifier() const { + return getFlag(fHasVirtualSpecifier); } bool isPure() const { return getFlag(fIsPure); @@ -878,8 +878,8 @@ private: void isConst(bool state) { setFlag(fIsConst, state); } - void isVirtual(bool state) { - setFlag(fIsVirtual, state); + void hasVirtualSpecifier(bool state) { + setFlag(fHasVirtualSpecifier, state); } void isPure(bool state) { setFlag(fIsPure, state); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 9c63ba52a..f90e88922 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -4836,7 +4836,7 @@ static void valueFlowFunctionReturn(TokenList *tokenlist, ErrorLogger *errorLogg &error); if (!error) { ValueFlow::Value v(result); - if (function->isVirtual()) + if (function->hasVirtualSpecifier()) v.setPossible(); else v.setKnown(); diff --git a/test/testclass.cpp b/test/testclass.cpp index 0e0a86d19..6bfa611cb 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -6828,6 +6828,23 @@ private: "int A::f() { return 1; }\n"); ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Virtual function 'f' is called from constructor 'A()' at line 3. Dynamic binding is not used.\n", errout.str()); + checkVirtualFunctionCall("class A : B {\n" + " int f() override;\n" + " A() {f();}\n" + "};\n" + "int A::f() { return 1; }\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (warning) Virtual function 'f' is called from constructor 'A()' at line 3. Dynamic binding is not used.\n", errout.str()); + + checkVirtualFunctionCall("class B {\n" + " virtual int f() = 0;\n" + "};\n" + "class A : B {\n" + " int f();\n" + " A() {f();}\n" + "};\n" + "int A::f() { return 1; }\n"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:5]: (warning) Virtual function 'f' is called from constructor 'A()' at line 6. Dynamic binding is not used.\n", errout.str()); + checkVirtualFunctionCall("class A\n" "{\n" " A() { A::f(); }\n" diff --git a/test/testsymboldatabase.cpp b/test/testsymboldatabase.cpp index 99a794a04..44c587547 100644 --- a/test/testsymboldatabase.cpp +++ b/test/testsymboldatabase.cpp @@ -4070,7 +4070,7 @@ private: const Token *f = db ? Token::findsimplematch(tokenizer.tokens(), "get_endpoint_url ( ) const noexcept ;") : nullptr; ASSERT(f != nullptr); ASSERT(f && f->function() && f->function()->token->linenr() == 2); - ASSERT(f && f->function() && f->function()->isVirtual()); + ASSERT(f && f->function() && f->function()->hasVirtualSpecifier()); ASSERT(f && f->function() && !f->function()->hasOverrideSpecifier()); ASSERT(f && f->function() && !f->function()->hasFinalSpecifier()); ASSERT(f && f->function() && f->function()->isConst()); @@ -4078,7 +4078,7 @@ private: f = db ? Token::findsimplematch(tokenizer.tokens(), "get_endpoint_url ( ) const noexcept override final ;") : nullptr; ASSERT(f != nullptr); ASSERT(f && f->function() && f->function()->token->linenr() == 5); - ASSERT(f && f->function() && f->function()->isVirtual()); + ASSERT(f && f->function() && f->function()->hasVirtualSpecifier()); ASSERT(f && f->function() && f->function()->hasOverrideSpecifier()); ASSERT(f && f->function() && f->function()->hasFinalSpecifier()); ASSERT(f && f->function() && f->function()->isConst());