From 016195dca9eb6f2ac50f5fd16d8972e6351a58e0 Mon Sep 17 00:00:00 2001 From: chrchr Date: Fri, 7 Jul 2023 17:13:14 +0200 Subject: [PATCH] Fix FP uselessOverride with shadowed member functions --- lib/checkclass.cpp | 47 ++++++++++++++++++++++++++++++++++++++++------ test/testclass.cpp | 10 ++++++++++ 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 5be6d2408c2..4bb99e3510f 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2893,9 +2893,15 @@ namespace { const Variable* parentClassVar; const Type::BaseInfo* parentClass; }; + struct DuplMemberFuncInfo { + DuplMemberFuncInfo(const Function* cf, const Function* pcf, const Type::BaseInfo* pc) : classFunc(cf), parentClassFunc(pcf), parentClass(pc) {} + const Function* classFunc; + const Function* parentClassFunc; + const Type::BaseInfo* parentClass; + }; } -static std::vector hasDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase) +static std::vector getDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase, bool skipPrivate = true) { std::vector results; for (const Type::BaseInfo &parentClassIt : typeBase->derivedFrom) { @@ -2908,12 +2914,38 @@ static std::vector hasDuplInheritedMembersRecursive(const Type* // Check if they have a member variable in common for (const Variable &classVarIt : typeCurrent->classScope->varlist) { for (const Variable &parentClassVarIt : parentClassIt.type->classScope->varlist) { - if (classVarIt.name() == parentClassVarIt.name() && !parentClassVarIt.isPrivate()) // Check if the class and its parent have a common variable + if (classVarIt.name() == parentClassVarIt.name() && (!parentClassVarIt.isPrivate() || !skipPrivate)) // Check if the class and its parent have a common variable results.emplace_back(&classVarIt, &parentClassVarIt, &parentClassIt); } } if (typeCurrent != parentClassIt.type) { - const auto recursive = hasDuplInheritedMembersRecursive(typeCurrent, parentClassIt.type); + const auto recursive = getDuplInheritedMembersRecursive(typeCurrent, parentClassIt.type, skipPrivate); + results.insert(results.end(), recursive.begin(), recursive.end()); + } + } + return results; +} + +static std::vector getDuplInheritedMemberFunctionsRecursive(const Type* typeCurrent, const Type* typeBase, bool skipPrivate = true) +{ + std::vector results; + for (const Type::BaseInfo &parentClassIt : typeBase->derivedFrom) { + // Check if there is info about the 'Base' class + if (!parentClassIt.type || !parentClassIt.type->classScope) + continue; + // Don't crash on recursive templates + if (parentClassIt.type == typeBase) + continue; + for (const Function& classFuncIt : typeCurrent->classScope->functionList) { + if (classFuncIt.isImplicitlyVirtual()) + continue; + for (const Function& parentClassFuncIt : parentClassIt.type->classScope->functionList) { + if (classFuncIt.name() == parentClassFuncIt.name() && (parentClassFuncIt.access != AccessControl::Private || !skipPrivate)) + results.emplace_back(&classFuncIt, &parentClassFuncIt, &parentClassIt); + } + } + if (typeCurrent != parentClassIt.type) { + const auto recursive = getDuplInheritedMemberFunctionsRecursive(typeCurrent, parentClassIt.type); results.insert(results.end(), recursive.begin(), recursive.end()); } } @@ -2922,7 +2954,7 @@ static std::vector hasDuplInheritedMembersRecursive(const Type* void CheckClass::checkDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase) { - const auto results = hasDuplInheritedMembersRecursive(typeCurrent, typeBase); + const auto results = getDuplInheritedMembersRecursive(typeCurrent, typeBase); for (const auto& r : results) { duplInheritedMembersError(r.classVar->nameToken(), r.parentClassVar->nameToken(), typeCurrent->name(), r.parentClass->type->name(), r.classVar->name(), @@ -3140,8 +3172,6 @@ void CheckClass::checkUselessOverride() continue; if (func.token->isExpandedMacro() || baseFunc->token->isExpandedMacro()) continue; - if (!classScope->definedType || !hasDuplInheritedMembersRecursive(classScope->definedType, classScope->definedType).empty()) // bailout for shadowed member variables - continue; if (baseFunc->functionScope) { bool isSameCode = compareTokenRanges(baseFunc->argDef, baseFunc->argDef->link(), func.argDef, func.argDef->link()); // function arguments if (isSameCode) { @@ -3149,6 +3179,11 @@ void CheckClass::checkUselessOverride() func.functionScope->bodyStart, func.functionScope->bodyEnd); if (isSameCode) { + // bailout for shadowed members + if (!classScope->definedType || + !getDuplInheritedMembersRecursive(classScope->definedType, classScope->definedType, /*skipPrivate*/ false).empty() || + !getDuplInheritedMemberFunctionsRecursive(classScope->definedType, classScope->definedType, /*skipPrivate*/ false).empty()) + continue; uselessOverrideError(baseFunc, &func, true); continue; } diff --git a/test/testclass.cpp b/test/testclass.cpp index 323f7bfd6a4..2f0ac6932bc 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8503,6 +8503,16 @@ class TestClass : public TestFixture { " int f() const override { return m; }\n" "};\n"); ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B {\n" + " int g() const;\n" + " virtual int f() const { return g(); }\n" + "};\n" + "struct D : B {\n" + " int g() const;\n" + " int f() const override { return g(); }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__)