From 9f73c6c289fed2901854e5eb9612b8aeae0fc99c Mon Sep 17 00:00:00 2001 From: chrchr Date: Thu, 6 Jul 2023 15:27:50 +0200 Subject: [PATCH 1/4] Fix uselessOverride FPs --- lib/checkclass.cpp | 39 ++++++++++++++++++++++++++++++--------- test/testclass.cpp | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 72 insertions(+), 13 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 2e784a327a2..defbbe9e726 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2886,8 +2886,9 @@ void CheckClass::checkDuplInheritedMembers() } } -void CheckClass::checkDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase) +static std::vector> hasDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase) { + 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) @@ -2898,16 +2899,30 @@ void CheckClass::checkDuplInheritedMembersRecursive(const Type* typeCurrent, con // 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 - duplInheritedMembersError(classVarIt.nameToken(), parentClassVarIt.nameToken(), - typeCurrent->name(), parentClassIt.type->name(), classVarIt.name(), - typeCurrent->classScope->type == Scope::eStruct, - parentClassIt.type->classScope->type == Scope::eStruct); - } + if (classVarIt.name() == parentClassVarIt.name() && !parentClassVarIt.isPrivate()) // Check if the class and its parent have a common variable + results.emplace_back(&classVarIt, &parentClassVarIt, &parentClassIt); } } - if (typeCurrent != parentClassIt.type) - checkDuplInheritedMembersRecursive(typeCurrent, parentClassIt.type); + if (typeCurrent != parentClassIt.type) { + const auto recursive = hasDuplInheritedMembersRecursive(typeCurrent, parentClassIt.type); + results.insert(results.end(), recursive.begin(), recursive.end()); + } + } + return results; +} + +void CheckClass::checkDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase) +{ + const auto results = hasDuplInheritedMembersRecursive(typeCurrent, typeBase); + for (const auto& r : results) { + if (const Variable* classVar = std::get<0>(r)) { + const Variable* parentClassVar = std::get<1>(r); + const Type::BaseInfo* parentClass = std::get<2>(r); + duplInheritedMembersError(classVar->nameToken(), parentClassVar->nameToken(), + typeCurrent->name(), parentClass->type->name(), classVar->name(), + typeCurrent->classScope->type == Scope::eStruct, + parentClass->type->classScope->type == Scope::eStruct); + } } } @@ -3084,6 +3099,8 @@ static bool compareTokenRanges(const Token* start1, const Token* end1, const Tok while (tok1 && tok2) { if (tok1->str() != tok2->str()) break; + if (tok1->str() == "this") + break; if (tok1 == end1 && tok2 == end2) { isEqual = true; break; @@ -3116,6 +3133,10 @@ void CheckClass::checkUselessOverride() return f.name() == func.name(); })) 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) { diff --git a/test/testclass.cpp b/test/testclass.cpp index 34a20b22625..65febc8acf9 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8347,12 +8347,20 @@ class TestClass : public TestFixture { const Settings settings = settingsBuilder().severity(Severity::style).build(); - Preprocessor preprocessor(settings); + // Raw tokens.. + std::vector files(1, "test.cpp"); + std::istringstream istr(code); + const simplecpp::TokenList tokens1(istr, files, files[0]); + + // Preprocess.. + simplecpp::TokenList tokens2(files); + std::map filedata; + simplecpp::preprocess(tokens2, tokens1, files, filedata, simplecpp::DUI()); // Tokenize.. - Tokenizer tokenizer(&settings, this, &preprocessor); - std::istringstream istr(code); - ASSERT_LOC(tokenizer.tokenize(istr, "test.cpp"), file, line); + Tokenizer tokenizer(&settings, this); + tokenizer.createTokens(std::move(tokens2)); + ASSERT_LOC(tokenizer.simplifyTokens1(""), file, line); // Check.. CheckClass checkClass(&tokenizer, &settings, this); @@ -8465,6 +8473,36 @@ class TestClass : public TestFixture { 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()); + + checkUselessOverride("struct B : std::exception {\n" + " virtual void f() { throw *this; }\n" + "};\n" + "struct D : B {\n" + " void f() override { throw *this; }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("#define MACRO virtual void f() {}\n" + "struct B {\n" + " MACRO\n" + "};\n" + "struct D : B {\n" + " MACRO\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); + + checkUselessOverride("struct B {\n" + " B() = default;\n" + " explicit B(int i) : m(i) {}\n" + " int m{};\n" + " virtual int f() const { return m; }\n" + "};\n" + "struct D : B {\n" + " explicit D(int i) : m(i) {}\n" + " int m{};\n" + " int f() const override { return m; }\n" + "};\n"); + ASSERT_EQUALS("", errout.str()); } #define checkUnsafeClassRefMember(code) checkUnsafeClassRefMember_(code, __FILE__, __LINE__) From 391d94f7ead679dc598451dbbd3647144f8e5106 Mon Sep 17 00:00:00 2001 From: chrchr Date: Thu, 6 Jul 2023 15:36:35 +0200 Subject: [PATCH 2/4] Use struct, redundant check --- lib/checkclass.cpp | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index defbbe9e726..5a4aca301e1 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2886,9 +2886,16 @@ void CheckClass::checkDuplInheritedMembers() } } -static std::vector> hasDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase) +namespace { + struct DuplMemberInfo { + const Variable* classVar{}, *parentClassVar{}; + const Type::BaseInfo* parentClass{}; + }; +} + +static std::vector hasDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase) { - std::vector> results; + 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) @@ -2900,7 +2907,7 @@ static std::vectorclassScope->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 - results.emplace_back(&classVarIt, &parentClassVarIt, &parentClassIt); + results.emplace_back(DuplMemberInfo{ &classVarIt, &parentClassVarIt, &parentClassIt }); } } if (typeCurrent != parentClassIt.type) { @@ -2915,14 +2922,10 @@ void CheckClass::checkDuplInheritedMembersRecursive(const Type* typeCurrent, con { const auto results = hasDuplInheritedMembersRecursive(typeCurrent, typeBase); for (const auto& r : results) { - if (const Variable* classVar = std::get<0>(r)) { - const Variable* parentClassVar = std::get<1>(r); - const Type::BaseInfo* parentClass = std::get<2>(r); - duplInheritedMembersError(classVar->nameToken(), parentClassVar->nameToken(), - typeCurrent->name(), parentClass->type->name(), classVar->name(), - typeCurrent->classScope->type == Scope::eStruct, - parentClass->type->classScope->type == Scope::eStruct); - } + duplInheritedMembersError(r.classVar->nameToken(), r.parentClassVar->nameToken(), + typeCurrent->name(), r.parentClass->type->name(), r.classVar->name(), + typeCurrent->classScope->type == Scope::eStruct, + r.parentClass->type->classScope->type == Scope::eStruct); } } From fe3fb5a7cea0f02a519d043809a1b6bc6571d37e Mon Sep 17 00:00:00 2001 From: chrchr Date: Thu, 6 Jul 2023 15:46:39 +0200 Subject: [PATCH 3/4] Format --- test/testclass.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/testclass.cpp b/test/testclass.cpp index 65febc8acf9..323f7bfd6a4 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8359,7 +8359,7 @@ class TestClass : public TestFixture { // Tokenize.. Tokenizer tokenizer(&settings, this); - tokenizer.createTokens(std::move(tokens2)); + tokenizer.createTokens(std::move(tokens2)); ASSERT_LOC(tokenizer.simplifyTokens1(""), file, line); // Check.. From b7af044a36cbb5852be0d87f47e661c09905bbf4 Mon Sep 17 00:00:00 2001 From: chrchr Date: Thu, 6 Jul 2023 16:41:24 +0200 Subject: [PATCH 4/4] Fix compilation --- lib/checkclass.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 5a4aca301e1..5be6d2408c2 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2888,8 +2888,10 @@ void CheckClass::checkDuplInheritedMembers() namespace { struct DuplMemberInfo { - const Variable* classVar{}, *parentClassVar{}; - const Type::BaseInfo* parentClass{}; + DuplMemberInfo(const Variable* cv, const Variable* pcv, const Type::BaseInfo* pc) : classVar(cv), parentClassVar(pcv), parentClass(pc) {} + const Variable* classVar; + const Variable* parentClassVar; + const Type::BaseInfo* parentClass; }; } @@ -2907,7 +2909,7 @@ static std::vector hasDuplInheritedMembersRecursive(const Type* 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 - results.emplace_back(DuplMemberInfo{ &classVarIt, &parentClassVarIt, &parentClassIt }); + results.emplace_back(&classVarIt, &parentClassVarIt, &parentClassIt); } } if (typeCurrent != parentClassIt.type) {