From c738627d15340b9bf0b1fb825020af2a9239cd79 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Fri, 7 Jul 2023 13:18:00 +0200 Subject: [PATCH] Fix uselessOverride FPs (#5223) --- lib/checkclass.cpp | 44 +++++++++++++++++++++++++++++++++++--------- test/testclass.cpp | 46 ++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 77 insertions(+), 13 deletions(-) diff --git a/lib/checkclass.cpp b/lib/checkclass.cpp index 2e784a327..5be6d2408 100644 --- a/lib/checkclass.cpp +++ b/lib/checkclass.cpp @@ -2886,8 +2886,18 @@ void CheckClass::checkDuplInheritedMembers() } } -void CheckClass::checkDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase) +namespace { + struct DuplMemberInfo { + 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; + }; +} + +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 +2908,26 @@ 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) { + 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); } } @@ -3084,6 +3104,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 +3138,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 34a20b226..323f7bfd6 100644 --- a/test/testclass.cpp +++ b/test/testclass.cpp @@ -8347,12 +8347,20 @@ private: 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 @@ private: 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__)