Skip to content

Commit 1d5166d

Browse files
danmardanmar
committed
CheckClass: Fix false negatives for uninitMemberVar
1 parent 49ffe80 commit 1d5166d

3 files changed

Lines changed: 87 additions & 46 deletions

File tree

‎lib/checkclass.cpp‎

Lines changed: 48 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -123,29 +123,6 @@ static bool isVclTypeInit(const Type *type)
123123
}
124124
return false;
125125
}
126-
127-
// Plain old C struct?
128-
static bool isPodStruct(const Scope *scope) {
129-
if (scope->type != Scope::ScopeType::eStruct && scope->type != Scope::ScopeType::eUnion)
130-
return false;
131-
if (!scope->functionList.empty())
132-
return false;
133-
for (const Variable& var: scope->varlist) {
134-
if (!var.valueType())
135-
return false;
136-
if (var.valueType()->isIntegral() || var.valueType()->pointer || var.valueType()->isFloat())
137-
continue;
138-
if (var.valueType()->typeScope && isPodStruct(var.valueType()->typeScope))
139-
continue;
140-
return false;
141-
}
142-
for (const Scope* childScope: scope->nestedList) {
143-
if (!isPodStruct(childScope))
144-
return false;
145-
}
146-
return true;
147-
}
148-
149126
//---------------------------------------------------------------------------
150127

151128
CheckClass::CheckClass(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger)
@@ -242,18 +219,42 @@ void CheckClass::constructors()
242219
// Mark all variables not used
243220
clearAllVar(usageList);
244221

245-
std::list<const Function *> callstack;
246-
initializeVarList(func, callstack, scope, usageList);
247-
248-
// Check if any variables are uninitialized
222+
// Variables with default initializers
249223
for (Usage &usage : usageList) {
250224
const Variable& var = *usage.var;
251225

252226
// check for C++11 initializer
253227
if (var.hasDefault() && func.type != Function::eOperatorEqual && func.type != Function::eCopyConstructor) { // variable still needs to be copied
254228
usage.init = true;
229+
}
230+
}
231+
232+
std::list<const Function *> callstack;
233+
initializeVarList(func, callstack, scope, usageList);
234+
235+
// Assign 1 union member => assign all union members
236+
for (const Usage &usage : usageList) {
237+
const Variable& var = *usage.var;
238+
if (!usage.assign && !usage.init)
255239
continue;
240+
const Scope* varScope1 = var.nameToken()->scope();
241+
if (varScope1->type == Scope::ScopeType::eUnion) {
242+
for (Usage &usage2 : usageList) {
243+
const Variable& var2 = *usage2.var;
244+
if (usage2.assign || usage2.init || var2.isStatic())
245+
continue;
246+
const Scope* varScope2 = var2.nameToken()->scope();
247+
if (varScope2->type == Scope::ScopeType::eStruct)
248+
varScope2 = varScope2->nestedIn;
249+
if (varScope1 == varScope2)
250+
usage2.assign = true;
251+
}
256252
}
253+
}
254+
255+
// Check if any variables are uninitialized
256+
for (const Usage &usage : usageList) {
257+
const Variable& var = *usage.var;
257258

258259
if (usage.assign || usage.init || var.isStatic())
259260
continue;
@@ -320,30 +321,17 @@ void CheckClass::constructors()
320321
const Scope *varType = var.typeScope();
321322
if (!varType || varType->type != Scope::eUnion) {
322323
const bool derived = scope != var.scope();
323-
// is the derived variable declared in a plain old C struct
324-
bool varScopeIsPodStruct = false;
325-
if (derived && scope->definedType && scope->definedType->derivedFrom.size() > 0) {
326-
for (const Scope* s = var.scope(); s; s = s ? s->nestedIn : nullptr) {
327-
for (const Type::BaseInfo& baseInfo: scope->definedType->derivedFrom) {
328-
if (s->definedType == baseInfo.type) {
329-
varScopeIsPodStruct = isPodStruct(s);
330-
s = nullptr;
331-
break;
332-
}
333-
}
334-
}
335-
}
336324
if (func.type == Function::eConstructor &&
337325
func.nestedIn && (func.nestedIn->numConstructors - func.nestedIn->numCopyOrMoveConstructors) > 1 &&
338326
func.argCount() == 0 && func.functionScope &&
339327
func.arg && func.arg->link()->next() == func.functionScope->bodyStart &&
340328
func.functionScope->bodyStart->link() == func.functionScope->bodyStart->next()) {
341329
// don't warn about user defined default constructor when there are other constructors
342-
if (printInconclusive && (!derived || !varScopeIsPodStruct))
330+
if (printInconclusive)
343331
uninitVarError(func.token, func.access == AccessControl::Private, func.type, var.scope()->className, var.name(), derived, true);
344332
} else if (missingCopy)
345333
missingMemberCopyError(func.token, func.type, var.scope()->className, var.name());
346-
else if (!derived || !varScopeIsPodStruct)
334+
else
347335
uninitVarError(func.token, func.access == AccessControl::Private, func.type, var.scope()->className, var.name(), derived, false);
348336
}
349337
}
@@ -678,6 +666,21 @@ void CheckClass::assignVar(std::vector<Usage> &usageList, nonneg int varid)
678666
}
679667
}
680668

669+
void CheckClass::assignVar(std::vector<Usage> &usageList, const Token* vartok)
670+
{
671+
if (vartok->varId() > 0) {
672+
assignVar(usageList, vartok->varId());
673+
return;
674+
}
675+
for (Usage& usage: usageList) {
676+
// FIXME: This is a workaround when varid is not set for a derived member
677+
if (usage.var->name() == vartok->str()) {
678+
usage.assign = true;
679+
return;
680+
}
681+
}
682+
}
683+
681684
void CheckClass::initVar(std::vector<Usage> &usageList, nonneg int varid)
682685
{
683686
for (Usage& usage: usageList) {
@@ -952,7 +955,8 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
952955
tok2 = tok2->next();
953956
if (tok2->str() == "&")
954957
tok2 = tok2->next();
955-
assignVar(usage, tok2->varId());
958+
if (isVariableChangedByFunctionCall(tok2, tok2->previous()->str() == "&", tok2->varId(), mSettings, nullptr))
959+
assignVar(usage, tok2->varId());
956960
}
957961
}
958962
}
@@ -991,7 +995,7 @@ void CheckClass::initializeVarList(const Function &func, std::list<const Functio
991995

992996
// Assignment of member variable?
993997
else if (Token::Match(ftok, "%name% =")) {
994-
assignVar(usage, ftok->varId());
998+
assignVar(usage, ftok);
995999
bool bailout = ftok->variable() && ftok->variable()->isReference();
9961000
const Token* tok2 = ftok->tokAt(2);
9971001
if (tok2->str() == "&") {

‎lib/checkclass.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,13 @@ class CPPCHECKLIB CheckClass : public Check {
352352
*/
353353
static void assignVar(std::vector<Usage> &usageList, nonneg int varid);
354354

355+
/**
356+
* @brief assign a variable in the varlist
357+
* @param usageList reference to usage vector
358+
* @param vartok variable token
359+
*/
360+
static void assignVar(std::vector<Usage> &usageList, const Token *vartok);
361+
355362
/**
356363
* @brief initialize a variable in the varlist
357364
* @param usageList reference to usage vector

‎test/testconstructors.cpp‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ class TestConstructors : public TestFixture {
126126
TEST_CASE(initvar_delegate); // ticket #4302
127127
TEST_CASE(initvar_delegate2);
128128
TEST_CASE(initvar_derived_class);
129-
TEST_CASE(initvar_derived_pod_struct); // #11101
129+
TEST_CASE(initvar_derived_pod_struct_with_union); // #11101
130130

131131
TEST_CASE(initvar_private_constructor); // BUG 2354171 - private constructor
132132
TEST_CASE(initvar_copy_constructor); // ticket #1611
@@ -1417,7 +1417,7 @@ class TestConstructors : public TestFixture {
14171417

14181418
}
14191419

1420-
void initvar_derived_pod_struct() {
1420+
void initvar_derived_pod_struct_with_union() {
14211421
check("struct S {\n"
14221422
" union {\n"
14231423
" unsigned short all;\n"
@@ -1433,6 +1433,24 @@ class TestConstructors : public TestFixture {
14331433
" C() { all = 0; tick = 0; }\n"
14341434
"};");
14351435
ASSERT_EQUALS("", errout.str());
1436+
1437+
check("struct S {\n"
1438+
" union {\n"
1439+
" unsigned short all;\n"
1440+
" struct {\n"
1441+
" unsigned char flag1;\n"
1442+
" unsigned char flag2;\n"
1443+
" };\n"
1444+
" };\n"
1445+
"};\n"
1446+
"\n"
1447+
"class C : public S {\n"
1448+
"public:\n"
1449+
" C() {}\n"
1450+
"};");
1451+
ASSERT_EQUALS("[test.cpp:13]: (warning) Member variable 'S::all' is not initialized in the constructor. Maybe it should be initialized directly in the class S?\n"
1452+
"[test.cpp:13]: (warning) Member variable 'S::flag1' is not initialized in the constructor. Maybe it should be initialized directly in the class S?\n"
1453+
"[test.cpp:13]: (warning) Member variable 'S::flag2' is not initialized in the constructor. Maybe it should be initialized directly in the class S?\n", errout.str());
14361454
}
14371455

14381456
void initvar_private_constructor() {
@@ -3634,6 +3652,18 @@ class TestConstructors : public TestFixture {
36343652
" void Init( Structure& S ) { S.C = 0; };\n"
36353653
"};");
36363654
ASSERT_EQUALS("", errout.str());
3655+
3656+
check("struct Structure {\n"
3657+
" int C;\n"
3658+
"};\n"
3659+
"\n"
3660+
"class A {\n"
3661+
" Structure B;\n"
3662+
"public:\n"
3663+
" A() { Init( B ); };\n"
3664+
" void Init(const Structure& S) { }\n"
3665+
"};");
3666+
ASSERT_EQUALS("[test.cpp:8]: (warning) Member variable 'A::B' is not initialized in the constructor.\n", errout.str());
36373667
}
36383668

36393669
void uninitSameClassName() {

0 commit comments

Comments
 (0)