Skip to content

Commit 514b7f4

Browse files
danmardanmar
committed
Fixed #9906 (False positive: constParameter (function pointer))
1 parent dae37f1 commit 514b7f4

5 files changed

Lines changed: 75 additions & 24 deletions

File tree

‎lib/checkother.cpp‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1349,7 +1349,7 @@ void CheckOther::checkConstVariable()
13491349
const Scope *scope = var->scope();
13501350
if (!scope->function)
13511351
continue;
1352-
Function *function = scope->function;
1352+
const Function *function = scope->function;
13531353
if (var->isArgument()) {
13541354
if (function->isImplicitlyVirtual() || function->templateDef)
13551355
continue;
@@ -1427,22 +1427,26 @@ void CheckOther::checkConstVariable()
14271427
continue;
14281428
}
14291429

1430-
constVariableError(var);
1430+
constVariableError(var, function);
14311431
}
14321432
}
14331433

1434-
void CheckOther::constVariableError(const Variable *var)
1434+
void CheckOther::constVariableError(const Variable *var, const Function *function)
14351435
{
1436-
const Token *tok = nullptr;
1437-
std::string name = "x";
1438-
std::string id = "Variable";
1439-
if (var) {
1440-
tok = var->nameToken();
1441-
name = var->name();
1442-
if (var->isArgument())
1443-
id = "Parameter";
1436+
const std::string vartype((var && var->isArgument()) ? "Parameter" : "Variable");
1437+
const std::string varname(var ? var->name() : std::string("x"));
1438+
1439+
ErrorPath errorPath;
1440+
std::string id = "const" + vartype;
1441+
std::string message = "$symbol:" + varname + "\n" + vartype + " '$symbol' can be declared with const";
1442+
errorPath.push_back(ErrorPathItem(var ? var->nameToken() : nullptr, message));
1443+
if (var && var->isArgument() && function && function->functionPointerUsage) {
1444+
errorPath.push_back(ErrorPathItem(function->functionPointerUsage, "You might need to cast the function pointer here"));
1445+
id += "Callback";
1446+
message += ". However it seems that '" + function->name() + "' is a callback function, if '$symbol' is declared with const you might also need to cast function pointer(s).";
14441447
}
1445-
reportError(tok, Severity::style, "const" + id, id + " '" + name + "' can be declared with const", CWE398, false);
1448+
1449+
reportError(errorPath, Severity::style, id.c_str(), message, CWE398, false);
14461450
}
14471451

14481452
//---------------------------------------------------------------------------

‎lib/checkother.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ namespace ValueFlow {
3838
class Settings;
3939
class Token;
4040
class Tokenizer;
41+
class Function;
4142
class Variable;
4243
class ErrorLogger;
4344

@@ -229,7 +230,7 @@ class CPPCHECKLIB CheckOther : public Check {
229230
void cstyleCastError(const Token *tok);
230231
void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive, bool toIsInt);
231232
void passedByValueError(const Token *tok, const std::string &parname, bool inconclusive);
232-
void constVariableError(const Variable *var);
233+
void constVariableError(const Variable *var, const Function *function);
233234
void constStatementError(const Token *tok, const std::string &type, bool inconclusive);
234235
void signedCharArrayIndexError(const Token *tok);
235236
void unknownSignCharArrayIndexError(const Token *tok);
@@ -301,7 +302,7 @@ class CPPCHECKLIB CheckOther : public Check {
301302
c.checkCastIntToCharAndBackError(nullptr, "func_name");
302303
c.cstyleCastError(nullptr);
303304
c.passedByValueError(nullptr, "parametername", false);
304-
c.constVariableError(nullptr);
305+
c.constVariableError(nullptr, nullptr);
305306
c.constStatementError(nullptr, "type", false);
306307
c.signedCharArrayIndexError(nullptr);
307308
c.unknownSignCharArrayIndexError(nullptr);

‎lib/symboldatabase.cpp‎

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1053,10 +1053,26 @@ void SymbolDatabase::createSymbolDatabaseSetFunctionPointers(bool firstPass)
10531053

10541054
// Set function call pointers
10551055
for (const Token* tok = mTokenizer->list.front(); tok != mTokenizer->list.back(); tok = tok->next()) {
1056-
if (!tok->function() && tok->varId() == 0 && tok->linkAt(1) && Token::Match(tok, "%name% (") && !isReservedName(tok->str())) {
1056+
if (tok->isName() && !tok->function() && tok->varId() == 0 && Token::Match(tok, "%name% [(,)>]") && !isReservedName(tok->str())) {
1057+
if (tok->next()->str() == ">" && !tok->next()->link())
1058+
continue;
1059+
1060+
if (tok->next()->str() != "(") {
1061+
const Token *start = tok;
1062+
while (Token::Match(start->tokAt(-2), "%name% ::"))
1063+
start = start->tokAt(-2);
1064+
if (!Token::Match(start->previous(), "[(,<]") && !Token::Match(start->tokAt(-2), "[(,<] &"))
1065+
continue;
1066+
}
1067+
10571068
const Function *function = findFunction(tok);
1058-
if (function)
1059-
const_cast<Token *>(tok)->function(function);
1069+
if (!function)
1070+
continue;
1071+
1072+
const_cast<Token *>(tok)->function(function);
1073+
1074+
if (tok->next()->str() != "(")
1075+
const_cast<Function *>(function)->functionPointerUsage = tok;
10601076
}
10611077
}
10621078

@@ -2063,6 +2079,7 @@ Function::Function(const Tokenizer *mTokenizer,
20632079
noexceptArg(nullptr),
20642080
throwArg(nullptr),
20652081
templateDef(nullptr),
2082+
functionPointerUsage(nullptr),
20662083
mFlags(0)
20672084
{
20682085
// operator function
@@ -2211,6 +2228,7 @@ Function::Function(const Token *tokenDef)
22112228
noexceptArg(nullptr),
22122229
throwArg(nullptr),
22132230
templateDef(nullptr),
2231+
functionPointerUsage(nullptr),
22142232
mFlags(0)
22152233
{
22162234
}
@@ -4602,10 +4620,7 @@ static std::string getTypeString(const Token *typeToken)
46024620

46034621
const Function* Scope::findFunction(const Token *tok, bool requireConst) const
46044622
{
4605-
// make sure this is a function call
4606-
const Token *end = tok->linkAt(1);
4607-
if (!end)
4608-
return nullptr;
4623+
const bool isCall = Token::Match(tok->next(), "(|{");
46094624

46104625
const std::vector<const Token *> arguments = getArguments(tok);
46114626

@@ -4617,7 +4632,7 @@ const Function* Scope::findFunction(const Token *tok, bool requireConst) const
46174632
auto addMatchingFunctions = [&](const Scope *scope) {
46184633
for (std::multimap<std::string, const Function *>::const_iterator it = scope->functionMap.find(tok->str()); it != scope->functionMap.cend() && it->first == tok->str(); ++it) {
46194634
const Function *func = it->second;
4620-
if (args == func->argCount() ||
4635+
if (!isCall || args == func->argCount() ||
46214636
(func->isVariadic() && args >= (func->argCount() - 1)) ||
46224637
(args < func->argCount() && args >= func->minArgCount())) {
46234638
matches.push_back(func);
@@ -4636,6 +4651,11 @@ const Function* Scope::findFunction(const Token *tok, bool requireConst) const
46364651
// check in base classes
46374652
findFunctionInBase(tok->str(), args, matches);
46384653

4654+
// Non-call => Do not match parameters
4655+
if (!isCall) {
4656+
return matches.empty() ? nullptr : matches[0];
4657+
}
4658+
46394659
const Function* fallback1Func = nullptr;
46404660
const Function* fallback2Func = nullptr;
46414661

@@ -4862,8 +4882,8 @@ const Function* SymbolDatabase::findFunction(const Token *tok) const
48624882
}
48634883

48644884
if (currScope) {
4865-
while (currScope && !(Token::Match(tok1, "%type% :: %any% (") ||
4866-
(Token::Match(tok1, "%type% <") && Token::Match(tok1->linkAt(1), "> :: %any% (")))) {
4885+
while (currScope && !(Token::Match(tok1, "%type% :: %name% [(),>]") ||
4886+
(Token::Match(tok1, "%type% <") && Token::Match(tok1->linkAt(1), "> :: %name% (")))) {
48674887
if (tok1->strAt(1) == "::")
48684888
tok1 = tok1->tokAt(2);
48694889
else

‎lib/symboldatabase.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -900,6 +900,7 @@ class CPPCHECKLIB Function {
900900
const Token *noexceptArg; ///< noexcept token
901901
const Token *throwArg; ///< throw token
902902
const Token *templateDef; ///< points to 'template <' before function
903+
const Token *functionPointerUsage; ///< function pointer usage
903904

904905
static bool argsMatch(const Scope *scope, const Token *first, const Token *second, const std::string &path, nonneg int path_length);
905906

‎test/testother.cpp‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ class TestOther : public TestFixture {
9898
TEST_CASE(passedByValue_externC);
9999

100100
TEST_CASE(constVariable);
101+
TEST_CASE(constParameterCallback);
101102

102103
TEST_CASE(switchRedundantAssignmentTest);
103104
TEST_CASE(switchRedundantOperationTest);
@@ -2522,6 +2523,30 @@ class TestOther : public TestFixture {
25222523
TODO_ASSERT_EQUALS("[test.cpp:16]: (style) Parameter 'i' can be declared with const\n", "", errout.str());
25232524
}
25242525

2526+
void constParameterCallback() {
2527+
check("int callback(std::vector<int>& x) { return x[0]; }\n"
2528+
"void f() { dostuff(callback); }");
2529+
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:2]: (style) Parameter 'x' can be declared with const. However it seems that 'callback' is a callback function, if 'x' is declared with const you might also need to cast function pointer(s).\n", errout.str());
2530+
2531+
// #9906
2532+
check("class EventEngine : public IEventEngine {\n"
2533+
"public:\n"
2534+
" EventEngine();\n"
2535+
"\n"
2536+
"private:\n"
2537+
" void signalEvent(ev::sig& signal, int revents);\n"
2538+
"};\n"
2539+
"\n"
2540+
"EventEngine::EventEngine() {\n"
2541+
" mSigWatcher.set<EventEngine, &EventEngine::signalEvent>(this);\n"
2542+
"}\n"
2543+
"\n"
2544+
"void EventEngine::signalEvent(ev::sig& signal, int revents) {\n"
2545+
" switch (signal.signum) {}\n"
2546+
"}");
2547+
ASSERT_EQUALS("[test.cpp:13] -> [test.cpp:10]: (style) Parameter 'signal' can be declared with const. However it seems that 'signalEvent' is a callback function, if 'signal' is declared with const you might also need to cast function pointer(s).\n", errout.str());
2548+
}
2549+
25252550
void switchRedundantAssignmentTest() {
25262551
check("void foo()\n"
25272552
"{\n"

0 commit comments

Comments
 (0)