From cacd6360d15b8e2ff787e37121b7b09af3156b76 Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 30 Jun 2023 10:29:18 +0200 Subject: [PATCH 1/3] 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__) From c0d72d781aaa4f17525866cedc50d20d1f95e257 Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 30 Jun 2023 11:33:38 +0200 Subject: [PATCH 2/3] Use helper function --- lib/checkclass.cpp | 49 ++++++++++++++++++++++++---------------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index a51dfb69e34..6049676709b 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -3077,6 +3077,23 @@ static const Token* getSingleFunctionCall(const Scope* scope) { return nullptr; } +static bool compareTokenRanges(const Token* start1, const Token* end1, const Token* start2, const Token* end2) { + const Token* tok1 = start1; + const Token* tok2 = start2; + bool isEqual = false; + while (tok1 && tok2) { + if (tok1->str() != tok2->str()) + break; + if (tok1 == end1 && tok2 == end2) { + isEqual = true; + break; + } + tok1 = tok1->next(); + tok2 = tok2->next(); + } + return isEqual; +} + void CheckClass::checkUselessOverride() { if (!mSettings->severity.isEnabled(Severity::style)) @@ -3088,32 +3105,17 @@ void CheckClass::checkUselessOverride() for (const Function& func : classScope->functionList) { if (!func.functionScope) continue; + if (func.hasFinalSpecifier()) + continue; const Function* baseFunc = func.getOverriddenFunction(); - if (!baseFunc || baseFunc->isPure()) + if (!baseFunc || baseFunc->isPure() || baseFunc->access != func.access) 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(); - } + bool isSameCode = compareTokenRanges(baseFunc->argDef, baseFunc->argDef->link(), func.argDef, func.argDef->link()); // function arguments 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(); - } + isSameCode = compareTokenRanges(baseFunc->functionScope->bodyStart, baseFunc->functionScope->bodyEnd, // function body + func.functionScope->bodyStart, func.functionScope->bodyEnd); + if (isSameCode) { uselessOverrideError(baseFunc, &func, true); continue; @@ -3125,7 +3127,8 @@ void CheckClass::checkUselessOverride() 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) { + if (funcArgs.size() != callArgs.size() || + !std::equal(funcArgs.begin(), funcArgs.end(), callArgs.begin(), [](const Token* t1, const Token* t2) { return t1->str() == t2->str(); })) continue; From aaf72710d0488e46b29d92ba938b2f2044d79ad2 Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 30 Jun 2023 13:11:40 +0200 Subject: [PATCH 3/3] Remove --- lib/valueflow.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 369a143af2f..2c6bdac7e31 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -7239,10 +7239,6 @@ struct MultiValueFlowAnalyzer : ValueFlowAnalyzer { return false; } - bool isGlobal() const override { - return false; - } - bool lowerToPossible() override { for (auto&& p:values) { if (p.second.isImpossible())