From cacd6360d15b8e2ff787e37121b7b09af3156b76 Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 30 Jun 2023 10:29:18 +0200 Subject: [PATCH] Detect code duplication in overrider --- lib/checkclass.cpp | 41 ++++++++++++++++++++++++++++++++++++++--- lib/checkclass.h | 2 +- test/testclass.cpp | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 4 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index fb8c7c8a19b..a51dfb69e34 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -3036,7 +3036,7 @@ void CheckClass::overrideError(const Function *funcInBase, const Function *funcI Certainty::normal); } -void CheckClass::uselessOverrideError(const Function *funcInBase, const Function *funcInDerived) +void CheckClass::uselessOverrideError(const Function *funcInBase, const Function *funcInDerived, bool isSameCode) { const std::string functionName = funcInDerived ? ((funcInDerived->isDestructor() ? "~" : "") + funcInDerived->name()) : ""; const std::string funcType = (funcInDerived && funcInDerived->isDestructor()) ? "destructor" : "function"; @@ -3047,9 +3047,15 @@ void CheckClass::uselessOverrideError(const Function *funcInBase, const Function errorPath.emplace_back(funcInDerived->tokenDef, char(std::toupper(funcType[0])) + funcType.substr(1) + " in derived class"); } + std::string errStr = "\nThe " + funcType + " '$symbol' overrides a " + funcType + " in a base class but "; + if (isSameCode) { + errStr += "is identical to the overridden function"; + } + else + errStr += "just delegates back to the base 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.", + "$symbol:" + functionName + + errStr, CWE(0U) /* Unknown CWE! */, Certainty::normal); } @@ -3085,6 +3091,35 @@ void CheckClass::checkUselessOverride() const Function* baseFunc = func.getOverriddenFunction(); if (!baseFunc || baseFunc->isPure()) continue; + if (baseFunc->functionScope) { + bool isSameCode = true; + const Token* baseTok = baseFunc->argDef; + const Token* funcTok = func.argDef; + while (baseTok != baseFunc->argDef->link() && funcTok != func.argDef->link()) { + if (baseTok->str() != funcTok->str()) { + isSameCode = false; + break; + } + baseTok = baseTok->next(); + funcTok = funcTok->next(); + } + if (isSameCode) { + baseTok = baseFunc->functionScope->bodyStart; + funcTok = func.functionScope->bodyStart; + while (baseTok != baseFunc->functionScope->bodyEnd && funcTok != func.functionScope->bodyEnd) { + if (baseTok->str() != funcTok->str()) { + isSameCode = false; + break; + } + baseTok = baseTok->next(); + funcTok = funcTok->next(); + } + if (isSameCode) { + uselessOverrideError(baseFunc, &func, true); + continue; + } + } + } if (const Token* const call = getSingleFunctionCall(func.functionScope)) { if (call->function() != baseFunc) continue; diff --git a/lib/checkclass.h b/lib/checkclass.h index 49c1cda74d1..3d0b1f8fdfc 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -231,7 +231,7 @@ class CPPCHECKLIB CheckClass : public Check { 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 uselessOverrideError(const Function *funcInBase, const Function *funcInDerived, bool isSameCode = false); 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 40a61e81ec4..59901c303e5 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8403,6 +8403,48 @@ class TestClass : public TestFixture { " int f() override { return B::f(); }\n" "};"); ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct S { virtual void f(); };\n" + "struct D : S {\n" + " void f() final { S::f(); }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct S {\n" + "protected:\n" + " virtual void f();\n" + "};\n" + "struct D : S {\n" + "public:\n" + " void f() override { S::f(); }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B { virtual void f(int, int, int) const; };\n" // #11799 + "struct D : B {\n" + " int m = 42;\n" + " void f(int a, int b, int c) const override;\n" + "};\n" + "void D::f(int a, int b, int c) const {\n" + " B::f(a, b, m);\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B {\n" + " virtual int f() { return 1; }\n" + " virtual int g() { return 7; }\n" + " virtual int h(int i, int j) { return i + j; }\n" + " virtual int j(int i, int j) { return i + j; }\n" + "};\n" + "struct D : B {\n" + " int f() override { return 2; }\n" + " int g() override { return 7; }\n" + " int h(int j, int i) override { return i + j; }\n" + " int j(int i, int j) override { return i + j; }\n" + "};"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:9]: (style) The function 'g' overrides a function in a base class but is identical to the overridden function\n" + "[test.cpp:5] -> [test.cpp:11]: (style) The function 'j' overrides a function in a base class but is identical to the overridden function\n", + errout.str()); } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__)