diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 2b49d061a..fb8c7c8a1 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -3036,6 +3036,70 @@ void CheckClass::overrideError(const Function *funcInBase, const Function *funcI Certainty::normal); } +void CheckClass::uselessOverrideError(const Function *funcInBase, const Function *funcInDerived) +{ + const std::string functionName = funcInDerived ? ((funcInDerived->isDestructor() ? "~" : "") + funcInDerived->name()) : ""; + const std::string funcType = (funcInDerived && funcInDerived->isDestructor()) ? "destructor" : "function"; + + ErrorPath errorPath; + if (funcInBase && funcInDerived) { + errorPath.emplace_back(funcInBase->tokenDef, "Virtual " + funcType + " in base class"); + errorPath.emplace_back(funcInDerived->tokenDef, char(std::toupper(funcType[0])) + funcType.substr(1) + " in derived class"); + } + + reportError(errorPath, Severity::style, "uselessOverride", + "$symbol:" + functionName + "\n" + "The " + funcType + " '$symbol' overrides a " + funcType + " in a base class but just delegates back to the base class.", + CWE(0U) /* Unknown CWE! */, + Certainty::normal); +} + +static const Token* getSingleFunctionCall(const Scope* scope) { + const Token* const start = scope->bodyStart->next(); + const Token* const end = Token::findsimplematch(start, ";", 1, scope->bodyEnd); + if (!end || end->next() != scope->bodyEnd) + return nullptr; + const Token* ftok = start; + if (ftok->str() == "return") + ftok = ftok->astOperand1(); + else { + while (Token::Match(ftok, "%name%|::")) + ftok = ftok->next(); + } + if (Token::simpleMatch(ftok, "(") && ftok->previous()->function()) + return ftok->previous(); + return nullptr; +} + +void CheckClass::checkUselessOverride() +{ + if (!mSettings->severity.isEnabled(Severity::style)) + return; + + for (const Scope* classScope : mSymbolDatabase->classAndStructScopes) { + if (!classScope->definedType || classScope->definedType->derivedFrom.size() != 1) + continue; + for (const Function& func : classScope->functionList) { + if (!func.functionScope) + continue; + const Function* baseFunc = func.getOverriddenFunction(); + if (!baseFunc || baseFunc->isPure()) + continue; + if (const Token* const call = getSingleFunctionCall(func.functionScope)) { + if (call->function() != baseFunc) + continue; + std::vector funcArgs = getArguments(func.tokenDef); + std::vector callArgs = getArguments(call); + if (!std::equal(funcArgs.begin(), funcArgs.end(), callArgs.begin(), [](const Token* t1, const Token* t2) { + return t1->str() == t2->str(); + })) + continue; + uselessOverrideError(baseFunc, &func); + } + } + } +} + void CheckClass::checkThisUseAfterFree() { if (!mSettings->severity.isEnabled(Severity::warning)) diff --git a/lib/checkclass.h b/lib/checkclass.h index a2303774d..49c1cda74 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -83,6 +83,7 @@ public: checkClass.checkExplicitConstructors(); checkClass.checkCopyCtorAndEqOperator(); checkClass.checkOverride(); + checkClass.checkUselessOverride(); checkClass.checkThisUseAfterFree(); checkClass.checkUnsafeClassRefMember(); } @@ -146,6 +147,9 @@ public: /** @brief Check that the override keyword is used when overriding virtual functions */ void checkOverride(); + /** @brief Check that the overriden function is not identical to the base function */ + void checkUselessOverride(); + /** @brief When "self pointer" is destroyed, 'this' might become invalid. */ void checkThisUseAfterFree(); @@ -227,6 +231,7 @@ private: void duplInheritedMembersError(const Token* tok1, const Token* tok2, const std::string &derivedName, const std::string &baseName, const std::string &variableName, bool derivedIsStruct, bool baseIsStruct); void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor); void overrideError(const Function *funcInBase, const Function *funcInDerived); + void uselessOverrideError(const Function *funcInBase, const Function *funcInDerived); void thisUseAfterFree(const Token *self, const Token *free, const Token *use); void unsafeClassRefMemberError(const Token *tok, const std::string &varname); void checkDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase); diff --git a/test/testclass.cpp b/test/testclass.cpp index 2a4be2d7c..40a61e81e 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -230,6 +230,8 @@ private: TEST_CASE(override1); TEST_CASE(overrideCVRefQualifiers); + TEST_CASE(uselessOverride); + TEST_CASE(thisUseAfterFree); TEST_CASE(unsafeClassRefMember); @@ -8338,6 +8340,70 @@ private: ASSERT_EQUALS("", errout.str()); } + #define checkUselessOverride(code) checkUselessOverride_(code, __FILE__, __LINE__) + void checkUselessOverride_(const char code[], const char* file, int line) { + // Clear the error log + errout.str(""); + + const Settings settings = settingsBuilder().severity(Severity::style).build(); + + Preprocessor preprocessor(settings); + + // Tokenize.. + Tokenizer tokenizer(&settings, this, &preprocessor); + std::istringstream istr(code); + ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line); + + // Check.. + CheckClass checkClass(&tokenizer, &settings, this); + (checkClass.checkUselessOverride)(); + } + + void uselessOverride() { + checkUselessOverride("struct B { virtual int f() { return 5; } };\n" // #11757 + "struct D : B {\n" + " int f() override { return B::f(); }\n" + "};"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3]: (style) The function 'f' overrides a function in a base class but just delegates back to the base class.\n", errout.str()); + + checkUselessOverride("struct B { virtual void f(); };\n" + "struct D : B {\n" + " void f() override { B::f(); }\n" + "};"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3]: (style) The function 'f' overrides a function in a base class but just delegates back to the base class.\n", errout.str()); + + checkUselessOverride("struct B { virtual int f() = 0; };\n" + "int B::f() { return 5; }\n" + "struct D : B {\n" + " int f() override { return B::f(); }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B { virtual int f(int i); };\n" + "struct D : B {\n" + " int f(int i) override { return B::f(i); }\n" + "};"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:3]: (style) The function 'f' overrides a function in a base class but just delegates back to the base class.\n", errout.str()); + + checkUselessOverride("struct B { virtual int f(int i); };\n" + "struct D : B {\n" + " int f(int i) override { return B::f(i + 1); }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B { virtual int f(int i, int j); };\n" + "struct D : B {\n" + " int f(int i, int j) override { return B::f(j, i); }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B { virtual int f(); };\n" + "struct I { virtual int f() = 0; };\n" + "struct D : B, I {\n" + " int f() override { return B::f(); }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__) void checkUnsafeClassRefMember_(const char code[], const char* file, int line) {