From 385f62e543af9348d83e7ac211208ba8b43696d4 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Tue, 13 Jun 2023 23:39:39 +0200 Subject: [PATCH 01/10] Fix #11757 Detect useless overriding functions --- lib/checkclass.cpp | 71 ++++++++++++++++++++++++++++++++++++++++++++++ lib/checkclass.h | 5 ++++ 2 files changed, 76 insertions(+) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 3a6bc79d074..09871289169 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -3017,6 +3017,77 @@ 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 Function* 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, "(")) + return ftok->previous()->function(); + 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 Function* call = getSingleFunctionCall(func.functionScope)) { + if (call != baseFunc) + continue; + const Token* callArg = call->arg->next(); + const Token* baseArg = baseFunc->arg->next(); + bool argsEqual = true; + while (callArg != call->arg->link()) { + if (callArg->str() != baseArg->str()) { + argsEqual = false; + break; + } + callArg = callArg->next(); + baseArg = baseArg->next(); + } + if (!argsEqual) + continue; + uselessOverrideError(baseFunc, &func); + } + } + } +} + void CheckClass::checkThisUseAfterFree() { if (!mSettings->severity.isEnabled(Severity::warning)) diff --git a/lib/checkclass.h b/lib/checkclass.h index a2303774d33..49c1cda74d1 100644 --- a/lib/checkclass.h +++ b/lib/checkclass.h @@ -83,6 +83,7 @@ class CPPCHECKLIB CheckClass : public Check { checkClass.checkExplicitConstructors(); checkClass.checkCopyCtorAndEqOperator(); checkClass.checkOverride(); + checkClass.checkUselessOverride(); checkClass.checkThisUseAfterFree(); checkClass.checkUnsafeClassRefMember(); } @@ -146,6 +147,9 @@ class CPPCHECKLIB CheckClass : public Check { /** @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 @@ 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 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); From 758a28ec2461a06a3ae1ed8a6117bfef058d7eda Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 14 Jun 2023 18:12:20 +0200 Subject: [PATCH 02/10] Improve, add tests --- lib/checkclass.cpp | 27 +++++++------------ test/testclass.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 17 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 09871289169..a5633e6261a 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -3035,7 +3035,7 @@ void CheckClass::uselessOverrideError(const Function *funcInBase, const Function Certainty::normal); } -static const Function* getSingleFunctionCall(const Scope* scope) { +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) @@ -3047,8 +3047,8 @@ static const Function* getSingleFunctionCall(const Scope* scope) { while (Token::Match(ftok, "%name%|::")) ftok = ftok->next(); } - if (Token::simpleMatch(ftok, "(")) - return ftok->previous()->function(); + if (Token::simpleMatch(ftok, "(") && ftok->previous()->function()) + return ftok->previous(); return nullptr; } @@ -3066,21 +3066,14 @@ void CheckClass::checkUselessOverride() const Function* baseFunc = func.getOverriddenFunction(); if (!baseFunc || baseFunc->isPure()) continue; - if (const Function* call = getSingleFunctionCall(func.functionScope)) { - if (call != baseFunc) + if (const Token* const call = getSingleFunctionCall(func.functionScope)) { + if (call->function() != baseFunc) continue; - const Token* callArg = call->arg->next(); - const Token* baseArg = baseFunc->arg->next(); - bool argsEqual = true; - while (callArg != call->arg->link()) { - if (callArg->str() != baseArg->str()) { - argsEqual = false; - break; - } - callArg = callArg->next(); - baseArg = baseArg->next(); - } - if (!argsEqual) + 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); } diff --git a/test/testclass.cpp b/test/testclass.cpp index 93d85a62376..43995ac64d6 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -228,6 +228,8 @@ class TestClass : public TestFixture { TEST_CASE(override1); TEST_CASE(overrideCVRefQualifiers); + + TEST_CASE(uselessOverride); TEST_CASE(thisUseAfterFree); @@ -8323,6 +8325,70 @@ class TestClass : public TestFixture { 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 {\m" + " int f() override { return B::f(); }\n" + "};"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (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 {\m" + " void f() override { B::f(); }\n" + "};"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (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) { From 976294a9d83adaa30cb1273f443e3d0691290b23 Mon Sep 17 00:00:00 2001 From: chrchr Date: Wed, 14 Jun 2023 18:48:23 +0200 Subject: [PATCH 03/10] Format --- test/testclass.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/testclass.cpp b/test/testclass.cpp index 43995ac64d6..beffe95c299 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -228,7 +228,7 @@ class TestClass : public TestFixture { TEST_CASE(override1); TEST_CASE(overrideCVRefQualifiers); - + TEST_CASE(uselessOverride); TEST_CASE(thisUseAfterFree); @@ -8346,16 +8346,16 @@ class TestClass : public TestFixture { void uselessOverride() { checkUselessOverride("struct B { virtual int f() { return 5; } };\n" // #11757 - "struct D : B {\m" + "struct D : B {\n" " int f() override { return B::f(); }\n" "};"); - ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) The function 'f' overrides a function in a base class but just delegates back to the base class.\n", errout.str()); + 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 {\m" + "struct D : B {\n" " void f() override { B::f(); }\n" "};"); - ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) The function 'f' overrides a function in a base class but just delegates back to the base class.\n", errout.str()); + 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" From f49073d53b36c9f1faa65502ecd54dae47104637 Mon Sep 17 00:00:00 2001 From: chrchr Date: Thu, 29 Jun 2023 18:45:53 +0200 Subject: [PATCH 04/10] Fix FPs: uselessOverride --- lib/checkclass.cpp | 7 +++++-- test/testclass.cpp | 26 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index fb8c7c8a19b..327830fb74e 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -3082,15 +3082,18 @@ 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 (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) { + 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; diff --git a/test/testclass.cpp b/test/testclass.cpp index 40a61e81ec4..24e039faaeb 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8403,6 +8403,32 @@ 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" + "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()); } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__) From e10cd1cf712905c85efea6840629b2dc492c7a6a Mon Sep 17 00:00:00 2001 From: chrchr Date: Thu, 29 Jun 2023 18:49:38 +0200 Subject: [PATCH 05/10] Comment --- test/testclass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/testclass.cpp b/test/testclass.cpp index 24e039faaeb..f9a96b41c8a 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8420,7 +8420,7 @@ class TestClass : public TestFixture { "};"); ASSERT_EQUALS("", errout.str()); - checkUselessOverride("struct B { virtual void f(int, int, int) const; };\n" + 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" From be9525d84dc874f5ee7abd730172bde5958ccd8e Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 30 Jun 2023 10:39:03 +0200 Subject: [PATCH 06/10] Fix #11803 FP uselessOverride - overloaded virtual member function --- lib/checkclass.cpp | 6 ++++++ test/testclass.cpp | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 327830fb74e..14ce236d793 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -3087,6 +3087,12 @@ void CheckClass::checkUselessOverride() const Function* baseFunc = func.getOverriddenFunction(); if (!baseFunc || baseFunc->isPure() || baseFunc->access != func.access) continue; + if (std::any_of(classScope->functionList.begin(), classScope->functionList.end(), [&func](const Function& f) { // check for overloads + if (&f == &func) + return false; + return f.name() == func.name(); + })) + continue; if (const Token* const call = getSingleFunctionCall(func.functionScope)) { if (call->function() != baseFunc) continue; diff --git a/test/testclass.cpp b/test/testclass.cpp index f9a96b41c8a..85f0c7f0a3e 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8429,6 +8429,17 @@ class TestClass : public TestFixture { " B::f(a, b, m);\n" "};"); ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B {\n" // #11803 + " virtual void f();\n" + " virtual void f(int i);\n" + "};\n" + "struct D : B {\n" + " void f() override { B::f(); }\n" + " void f(int i) override;\n" + " void g() { f(); }\n" + "};"); + ASSERT_EQUALS("", errout.str()); } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__) From d64aef25818dafc605185933bed53c6f1ba669fa Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Sat, 1 Jul 2023 01:54:41 +0200 Subject: [PATCH 07/10] Fix FP uselessOverride --- lib/symboldatabase.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 7f09400345c..6fb5acbec75 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -4388,13 +4388,13 @@ const Function * Function::getOverriddenFunctionRecursive(const ::Type* baseType auto range = parent->functionMap.equal_range(tokenDef->str()); for (std::multimap::const_iterator it = range.first; it != range.second; ++it) { const Function * func = it->second; - if (func->hasVirtualSpecifier()) { // Base is virtual and of same name + if (func->isImplicitlyVirtual()) { // Base is virtual and of same name const Token *temp1 = func->tokenDef->previous(); const Token *temp2 = tokenDef->previous(); bool match = true; // check for matching return parameters - while (temp1->str() != "virtual") { + while (!Token::Match(temp1, "virtual|{|}|;")) { if (temp1->str() != temp2->str() && !(temp1->str() == derivedFromType->name() && temp2->str() == baseType->name())) { From 5403446772345a59cd0bb836e361208b0f8fa498 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Sat, 1 Jul 2023 02:36:41 +0200 Subject: [PATCH 08/10] Add test --- test/testclass.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/testclass.cpp b/test/testclass.cpp index 85f0c7f0a3e..4cf8dc3e86f 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8440,6 +8440,15 @@ class TestClass : public TestFixture { " void g() { f(); }\n" "};"); ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B { virtual void f(); };\n" // #11808 + "struct D : B { void f() override {} };\n" + "struct D2 : D {\n" + " void f() override {\n" + " Base::method();\n" + " }\n" + "};"); + ASSERT_EQUALS("", errout.str()); } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__) From 39f2e98c62653bd0c043dea9c4dc0e86f5fdcd74 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Sat, 1 Jul 2023 13:25:55 +0200 Subject: [PATCH 09/10] Merge --- test/testclass.cpp | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/test/testclass.cpp b/test/testclass.cpp index 4cf8dc3e86f..f9a96b41c8a 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8429,26 +8429,6 @@ class TestClass : public TestFixture { " B::f(a, b, m);\n" "};"); ASSERT_EQUALS("", errout.str()); - - checkUselessOverride("struct B {\n" // #11803 - " virtual void f();\n" - " virtual void f(int i);\n" - "};\n" - "struct D : B {\n" - " void f() override { B::f(); }\n" - " void f(int i) override;\n" - " void g() { f(); }\n" - "};"); - ASSERT_EQUALS("", errout.str()); - - checkUselessOverride("struct B { virtual void f(); };\n" // #11808 - "struct D : B { void f() override {} };\n" - "struct D2 : D {\n" - " void f() override {\n" - " Base::method();\n" - " }\n" - "};"); - ASSERT_EQUALS("", errout.str()); } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__) From 5d803826dd4d08310b34270def5f05c0ff419cbe Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Sat, 1 Jul 2023 13:33:41 +0200 Subject: [PATCH 10/10] Fix merge --- test/testclass.cpp | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/testclass.cpp b/test/testclass.cpp index f9a96b41c8a..995e343835d 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8429,6 +8429,26 @@ class TestClass : public TestFixture { " B::f(a, b, m);\n" "};"); ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B {\n" // #11803 + " virtual void f();\n" + " virtual void f(int i);\n" + "};\n" + "struct D : B {\n" + " void f() override { B::f(); }\n" + " void f(int i) override;\n" + " void g() { f(); }\n" + "};"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B { virtual void f(); };\n" // #11808 + "struct D : B { void f() override {} };\n" + "struct D2 : D {\n" + " void f() override {\n" + " B::f();\n" + " }\n" + "};"); + ASSERT_EQUALS("", errout.str()); } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__)