Skip to content

Fix uselessOverride FPs #5223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
chrchr-github merged 4 commits into from
Jul 7, 2023
Merged

Fix uselessOverride FPs #5223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9f73c6c
Fix uselessOverride FPs
chrchr-github Jul 6, 2023
391d94f
Use struct, redundant check
chrchr-github Jul 6, 2023
fe3fb5a
Format
chrchr-github Jul 6, 2023
b7af044
Fix compilation
chrchr-github Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 35 additions & 9 deletions lib/checkclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<DuplMemberInfo> hasDuplInheritedMembersRecursive(const Type* typeCurrent, const Type* typeBase)
{
std::vector<DuplMemberInfo> results;
for (const Type::BaseInfo &parentClassIt : typeBase->derivedFrom) {
// Check if there is info about the 'Base' class
if (!parentClassIt.type || !parentClassIt.type->classScope)
Expand All @@ -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);
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
46 changes: 42 additions & 4 deletions test/testclass.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8347,12 +8347,20 @@ class TestClass : public TestFixture {

const Settings settings = settingsBuilder().severity(Severity::style).build();

Preprocessor preprocessor(settings);
// Raw tokens..
std::vector<std::string> files(1, "test.cpp");
std::istringstream istr(code);
const simplecpp::TokenList tokens1(istr, files, files[0]);

// Preprocess..
simplecpp::TokenList tokens2(files);
std::map<std::string, simplecpp::TokenList*> 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);
Expand Down Expand Up @@ -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__)
Expand Down