Skip to content

Commit 1046ca2

Browse files
danmardanmar
committed
Improve check: Warn about virtual function calls in constructor/destructor
1 parent e492932 commit 1046ca2

3 files changed

Lines changed: 284 additions & 253 deletions

File tree

‎lib/checkclass.cpp‎

Lines changed: 102 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,6 @@ static const char * getFunctionTypeName(Function::Type type)
6464
return "";
6565
}
6666

67-
static bool isPureWithoutBody(Function const & func)
68-
{
69-
return func.isPure() && !func.hasBody();
70-
}
71-
7267
//---------------------------------------------------------------------------
7368

7469
CheckClass::CheckClass(const Tokenizer *tokenizer, const Settings *settings, ErrorLogger *errorLogger)
@@ -2200,122 +2195,143 @@ void CheckClass::selfInitializationError(const Token* tok, const std::string& va
22002195

22012196

22022197
//---------------------------------------------------------------------------
2203-
// Check for pure virtual function calls
2198+
// Check for virtual function calls in constructor/destructor
22042199
//---------------------------------------------------------------------------
22052200

2206-
void CheckClass::checkPureVirtualFunctionCall()
2201+
void CheckClass::checkVirtualFunctionCallInConstructor()
22072202
{
22082203
if (! _settings->isEnabled(Settings::WARNING))
22092204
return;
22102205
const std::size_t functions = symbolDatabase->functionScopes.size();
2211-
std::map<const Function *, std::list<const Token *> > callsPureVirtualFunctionMap;
2206+
std::map<const Function *, std::list<const Token *> > virtualFunctionCallsMap;
22122207
for (std::size_t i = 0; i < functions; ++i) {
22132208
const Scope * scope = symbolDatabase->functionScopes[i];
22142209
if (scope->function == nullptr || !scope->function->hasBody() ||
22152210
!(scope->function->isConstructor() ||
22162211
scope->function->isDestructor()))
22172212
continue;
22182213

2219-
const std::list<const Token *> & pureVirtualFunctionCalls=callsPureVirtualFunction(*scope->function,callsPureVirtualFunctionMap);
2220-
for (std::list<const Token *>::const_iterator pureCallIter=pureVirtualFunctionCalls.begin();
2221-
pureCallIter!=pureVirtualFunctionCalls.end();
2222-
++pureCallIter) {
2223-
const Token & pureCall=**pureCallIter;
2224-
std::list<const Token *> pureFuncStack;
2225-
pureFuncStack.push_back(&pureCall);
2226-
getFirstPureVirtualFunctionCallStack(callsPureVirtualFunctionMap, pureCall, pureFuncStack);
2227-
if (!pureFuncStack.empty())
2228-
callsPureVirtualFunctionError(*scope->function, pureFuncStack, pureFuncStack.back()->str());
2214+
const std::list<const Token *> & virtualFunctionCalls = getVirtualFunctionCalls(*scope->function, virtualFunctionCallsMap);
2215+
for (std::list<const Token *>::const_iterator it = virtualFunctionCalls.begin(); it != virtualFunctionCalls.end(); ++it) {
2216+
const Token * callToken = *it;
2217+
std::list<const Token *> callstack;
2218+
callstack.push_back(callToken);
2219+
getFirstVirtualFunctionCallStack(virtualFunctionCallsMap, callToken, callstack);
2220+
if (callstack.empty())
2221+
continue;
2222+
if (callstack.back()->function()->isPure())
2223+
pureVirtualFunctionCallInConstructorError(scope->function, callstack, callstack.back()->str());
2224+
else
2225+
virtualFunctionCallInConstructorError(scope->function, callstack, callstack.back()->str());
22292226
}
22302227
}
22312228
}
22322229

2233-
const std::list<const Token *> & CheckClass::callsPureVirtualFunction(const Function & function,
2234-
std::map<const Function *, std::list<const Token *> > & callsPureVirtualFunctionMap)
2235-
{
2236-
std::pair<std::map<const Function *, std::list<const Token *> >::iterator, bool > found =
2237-
callsPureVirtualFunctionMap.insert(std::pair<const Function *, std::list< const Token *> >(&function, std::list<const Token *>()));
2238-
std::list<const Token *> & pureFunctionCalls = found.first->second;
2239-
if (found.second) {
2240-
if (function.hasBody()) {
2241-
for (const Token *tok = function.arg->link();
2242-
tok && tok != function.functionScope->classEnd;
2243-
tok = tok->next()) {
2244-
if (function.type != Function::eConstructor &&
2245-
function.type != Function::eCopyConstructor &&
2246-
function.type != Function::eMoveConstructor &&
2247-
function.type != Function::eDestructor) {
2248-
if ((Token::simpleMatch(tok, ") {") &&
2249-
tok->link() &&
2250-
Token::Match(tok->link()->previous(), "if|switch")) ||
2251-
Token::simpleMatch(tok, "else {")
2252-
) {
2253-
// Assume pure virtual function call is prevented by "if|else|switch" condition
2254-
tok = tok->linkAt(1);
2255-
continue;
2256-
}
2257-
}
2258-
if (tok->scope()->type == Scope::eLambda)
2259-
tok = tok->scope()->classEnd->next();
2260-
2261-
const Function * callFunction = tok->function();
2262-
if (!callFunction ||
2263-
function.nestedIn != callFunction->nestedIn ||
2264-
(tok->previous() && tok->previous()->str() == "."))
2265-
continue;
2230+
const std::list<const Token *> & CheckClass::getVirtualFunctionCalls(const Function & function,
2231+
std::map<const Function *, std::list<const Token *> > & virtualFunctionCallsMap)
2232+
{
2233+
const std::map<const Function *, std::list<const Token *> >::const_iterator found = virtualFunctionCallsMap.find(&function);
2234+
if (found != virtualFunctionCallsMap.end())
2235+
return found->second;
22662236

2267-
if (tok->previous() &&
2268-
tok->previous()->str() == "(") {
2269-
const Token * prev = tok->previous();
2270-
if (prev->previous() &&
2271-
(_settings->library.ignorefunction(tok->str())
2272-
|| _settings->library.ignorefunction(prev->previous()->str())))
2273-
continue;
2274-
}
2237+
virtualFunctionCallsMap[&function] = std::list<const Token *>();
2238+
std::list<const Token *> & virtualFunctionCalls = virtualFunctionCallsMap.find(&function)->second;
22752239

2276-
if (isPureWithoutBody(*callFunction)) {
2277-
pureFunctionCalls.push_back(tok);
2278-
continue;
2279-
}
2240+
if (!function.hasBody())
2241+
return virtualFunctionCalls;
22802242

2281-
const std::list<const Token *> & pureFunctionCallsOfTok = callsPureVirtualFunction(*callFunction,
2282-
callsPureVirtualFunctionMap);
2283-
if (!pureFunctionCallsOfTok.empty()) {
2284-
pureFunctionCalls.push_back(tok);
2285-
continue;
2286-
}
2243+
for (const Token *tok = function.arg->link(); tok != function.functionScope->classEnd; tok = tok->next()) {
2244+
if (function.type != Function::eConstructor &&
2245+
function.type != Function::eCopyConstructor &&
2246+
function.type != Function::eMoveConstructor &&
2247+
function.type != Function::eDestructor) {
2248+
if ((Token::simpleMatch(tok, ") {") && tok->link() && Token::Match(tok->link()->previous(), "if|switch")) ||
2249+
Token::simpleMatch(tok, "else {")) {
2250+
// Assume pure virtual function call is prevented by "if|else|switch" condition
2251+
tok = tok->linkAt(1);
2252+
continue;
22872253
}
22882254
}
2255+
if (tok->scope()->type == Scope::eLambda)
2256+
tok = tok->scope()->classEnd->next();
2257+
2258+
const Function * callFunction = tok->function();
2259+
if (!callFunction ||
2260+
function.nestedIn != callFunction->nestedIn ||
2261+
(tok->previous() && tok->previous()->str() == "."))
2262+
continue;
2263+
2264+
if (tok->previous() &&
2265+
tok->previous()->str() == "(") {
2266+
const Token * prev = tok->previous();
2267+
if (prev->previous() &&
2268+
(_settings->library.ignorefunction(tok->str())
2269+
|| _settings->library.ignorefunction(prev->previous()->str())))
2270+
continue;
2271+
}
2272+
2273+
if (callFunction->isVirtual()) {
2274+
virtualFunctionCalls.push_back(tok);
2275+
continue;
2276+
}
2277+
2278+
const std::list<const Token *> & virtualFunctionCallsOfTok = getVirtualFunctionCalls(*callFunction, virtualFunctionCallsMap);
2279+
if (!virtualFunctionCallsOfTok.empty())
2280+
virtualFunctionCalls.push_back(tok);
22892281
}
2290-
return pureFunctionCalls;
2282+
return virtualFunctionCalls;
22912283
}
22922284

2293-
void CheckClass::getFirstPureVirtualFunctionCallStack(
2294-
std::map<const Function *, std::list<const Token *> > & callsPureVirtualFunctionMap,
2295-
const Token & pureCall,
2296-
std::list<const Token *> & pureFuncStack)
2285+
void CheckClass::getFirstVirtualFunctionCallStack(
2286+
std::map<const Function *, std::list<const Token *> > & virtualFunctionCallsMap,
2287+
const Token * callToken,
2288+
std::list<const Token *> & callstack)
22972289
{
2298-
if (isPureWithoutBody(*pureCall.function())) {
2299-
pureFuncStack.push_back(pureCall.function()->token);
2290+
const Function *callFunction = callToken->function();
2291+
if (callFunction->isVirtual() && (!callFunction->isPure() || !callFunction->hasBody())) {
2292+
callstack.push_back(callFunction->token);
23002293
return;
23012294
}
2302-
std::map<const Function *, std::list<const Token *> >::const_iterator found = callsPureVirtualFunctionMap.find(pureCall.function());
2303-
if (found == callsPureVirtualFunctionMap.end() ||
2304-
found->second.empty()) {
2305-
pureFuncStack.clear();
2295+
std::map<const Function *, std::list<const Token *> >::const_iterator found = virtualFunctionCallsMap.find(callFunction);
2296+
if (found == virtualFunctionCallsMap.end() || found->second.empty()) {
2297+
callstack.clear();
23062298
return;
23072299
}
2308-
const Token & firstPureCall = **found->second.begin();
2309-
pureFuncStack.push_back(&firstPureCall);
2310-
getFirstPureVirtualFunctionCallStack(callsPureVirtualFunctionMap, firstPureCall, pureFuncStack);
2300+
const Token * firstCall = *found->second.begin();
2301+
callstack.push_back(firstCall);
2302+
getFirstVirtualFunctionCallStack(virtualFunctionCallsMap, firstCall, callstack);
23112303
}
23122304

2313-
void CheckClass::callsPureVirtualFunctionError(
2314-
const Function & scopeFunction,
2305+
void CheckClass::virtualFunctionCallInConstructorError(
2306+
const Function * scopeFunction,
2307+
const std::list<const Token *> & tokStack,
2308+
const std::string &funcname)
2309+
{
2310+
const char * scopeFunctionTypeName = scopeFunction ? getFunctionTypeName(scopeFunction->type) : "constructor";
2311+
2312+
ErrorPath errorPath;
2313+
for (std::list<const Token *>::const_iterator it = tokStack.begin(); it != tokStack.end(); ++it)
2314+
errorPath.push_back(ErrorPathItem(*it, "Calling " + (*it)->str()));
2315+
if (!errorPath.empty())
2316+
errorPath.back().second = funcname + " is a virtual method";
2317+
2318+
reportError(errorPath, Severity::warning, "virtualCallInConstructor", "Call of virtual function '" + funcname + "' in " + scopeFunctionTypeName + ".\n"
2319+
"Call of pure virtual function '" + funcname + "' in " + scopeFunctionTypeName + ". Dynamic binding is not used.", CWE(0U), false);
2320+
}
2321+
2322+
void CheckClass::pureVirtualFunctionCallInConstructorError(
2323+
const Function * scopeFunction,
23152324
const std::list<const Token *> & tokStack,
23162325
const std::string &purefuncname)
23172326
{
2318-
const char * scopeFunctionTypeName = getFunctionTypeName(scopeFunction.type);
2327+
const char * scopeFunctionTypeName = scopeFunction ? getFunctionTypeName(scopeFunction->type) : "constructor";
2328+
2329+
ErrorPath errorPath;
2330+
for (std::list<const Token *>::const_iterator it = tokStack.begin(); it != tokStack.end(); ++it)
2331+
errorPath.push_back(ErrorPathItem(*it, "Calling " + (*it)->str()));
2332+
if (!errorPath.empty())
2333+
errorPath.back().second = purefuncname + " is a pure virtual method without body";
2334+
23192335
reportError(tokStack, Severity::warning, "pureVirtualCall", "Call of pure virtual function '" + purefuncname + "' in " + scopeFunctionTypeName + ".\n"
23202336
"Call of pure virtual function '" + purefuncname + "' in " + scopeFunctionTypeName + ". The call will fail during runtime.", CWE(0U), false);
23212337
}

‎lib/checkclass.h‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ class CPPCHECKLIB CheckClass : public Check {
8585
checkClass.virtualDestructor();
8686
checkClass.checkConst();
8787
checkClass.copyconstructors();
88-
checkClass.checkPureVirtualFunctionCall();
88+
checkClass.checkVirtualFunctionCallInConstructor();
8989

9090
checkClass.checkDuplInheritedMembers();
9191
checkClass.checkExplicitConstructors();
@@ -143,8 +143,8 @@ class CPPCHECKLIB CheckClass : public Check {
143143

144144
void copyconstructors();
145145

146-
/** @brief call of pure virtual function */
147-
void checkPureVirtualFunctionCall();
146+
/** @brief call of virtual function in constructor/destructor */
147+
void checkVirtualFunctionCallInConstructor();
148148

149149
/** @brief Check duplicated inherited members */
150150
void checkDuplInheritedMembers();
@@ -184,7 +184,8 @@ class CPPCHECKLIB CheckClass : public Check {
184184
void initializerListError(const Token *tok1,const Token *tok2, const std::string & classname, const std::string &varname);
185185
void suggestInitializationList(const Token *tok, const std::string& varname);
186186
void selfInitializationError(const Token* tok, const std::string& varname);
187-
void callsPureVirtualFunctionError(const Function & scopeFunction, const std::list<const Token *> & tokStack, const std::string &purefuncname);
187+
void pureVirtualFunctionCallInConstructorError(const Function * scopeFunction, const std::list<const Token *> & tokStack, const std::string &purefuncname);
188+
void virtualFunctionCallInConstructorError(const Function * scopeFunction, const std::list<const Token *> & tokStack, const std::string &funcname);
188189
void duplInheritedMembersError(const Token* tok1, const Token* tok2, const std::string &derivedname, const std::string &basename, const std::string &variablename, bool derivedIsStruct, bool baseIsStruct);
189190
void copyCtorAndEqOperatorError(const Token *tok, const std::string &classname, bool isStruct, bool hasCopyCtor);
190191
void unsafeClassDivZeroError(const Token *tok, const std::string &className, const std::string &methodName, const std::string &varName);
@@ -219,6 +220,8 @@ class CPPCHECKLIB CheckClass : public Check {
219220
c.duplInheritedMembersError(nullptr, nullptr, "class", "class", "variable", false, false);
220221
c.copyCtorAndEqOperatorError(nullptr, "class", false, false);
221222
c.unsafeClassDivZeroError(nullptr, "Class", "dostuff", "x");
223+
c.pureVirtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
224+
c.virtualFunctionCallInConstructorError(nullptr, std::list<const Token *>(), "f");
222225
}
223226

224227
static std::string myName() {
@@ -317,22 +320,22 @@ class CPPCHECKLIB CheckClass : public Check {
317320
/**
318321
* @brief gives a list of tokens where pure virtual functions are called directly or indirectly
319322
* @param function function to be checked
320-
* @param callsPureVirtualFunctionMap map of results for already checked functions
323+
* @param virtualFunctionCallsMap map of results for already checked functions
321324
* @return list of tokens where pure virtual functions are called
322325
*/
323-
const std::list<const Token *> & callsPureVirtualFunction(
326+
const std::list<const Token *> & getVirtualFunctionCalls(
324327
const Function & function,
325-
std::map<const Function *, std::list<const Token *> > & callsPureVirtualFunctionMap);
328+
std::map<const Function *, std::list<const Token *> > & virtualFunctionCallsMap);
326329

327330
/**
328331
* @brief looks for the first pure virtual function call stack
329-
* @param callsPureVirtualFunctionMap map of results obtained from callsPureVirtualFunction
330-
* @param pureCall token where pure virtual function is called directly or indirectly
332+
* @param virtualFunctionCallsMap map of results obtained from getVirtualFunctionCalls
333+
* @param callToken token where pure virtual function is called directly or indirectly
331334
* @param[in,out] pureFuncStack list to append the stack
332335
*/
333-
void getFirstPureVirtualFunctionCallStack(
334-
std::map<const Function *, std::list<const Token *> > & callsPureVirtualFunctionMap,
335-
const Token & pureCall,
336+
void getFirstVirtualFunctionCallStack(
337+
std::map<const Function *, std::list<const Token *> > & virtualFunctionCallsMap,
338+
const Token *callToken,
336339
std::list<const Token *> & pureFuncStack);
337340

338341
static bool canNotCopy(const Scope *scope);

0 commit comments

Comments
 (0)