diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 1d918561d..da1c5a34f 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1349,7 +1349,7 @@ void CheckOther::checkConstVariable() const Scope *scope = var->scope(); if (!scope->function) continue; - Function *function = scope->function; + const Function *function = scope->function; if (var->isArgument()) { if (function->isImplicitlyVirtual() || function->templateDef) continue; @@ -1427,22 +1427,26 @@ void CheckOther::checkConstVariable() continue; } - constVariableError(var); + constVariableError(var, function); } } -void CheckOther::constVariableError(const Variable *var) +void CheckOther::constVariableError(const Variable *var, const Function *function) { - const Token *tok = nullptr; - std::string name = "x"; - std::string id = "Variable"; - if (var) { - tok = var->nameToken(); - name = var->name(); - if (var->isArgument()) - id = "Parameter"; + const std::string vartype((var && var->isArgument()) ? "Parameter" : "Variable"); + const std::string varname(var ? var->name() : std::string("x")); + + ErrorPath errorPath; + std::string id = "const" + vartype; + std::string message = "$symbol:" + varname + "\n" + vartype + " '$symbol' can be declared with const"; + errorPath.push_back(ErrorPathItem(var ? var->nameToken() : nullptr, message)); + if (var && var->isArgument() && function && function->functionPointerUsage) { + errorPath.push_back(ErrorPathItem(function->functionPointerUsage, "You might need to cast the function pointer here")); + id += "Callback"; + 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)."; } - reportError(tok, Severity::style, "const" + id, id + " '" + name + "' can be declared with const", CWE398, false); + + reportError(errorPath, Severity::style, id.c_str(), message, CWE398, false); } //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index 3029d3734..0e950d5fe 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -38,6 +38,7 @@ namespace ValueFlow { class Settings; class Token; class Tokenizer; +class Function; class Variable; class ErrorLogger; @@ -229,7 +230,7 @@ private: void cstyleCastError(const Token *tok); void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive, bool toIsInt); void passedByValueError(const Token *tok, const std::string &parname, bool inconclusive); - void constVariableError(const Variable *var); + void constVariableError(const Variable *var, const Function *function); void constStatementError(const Token *tok, const std::string &type, bool inconclusive); void signedCharArrayIndexError(const Token *tok); void unknownSignCharArrayIndexError(const Token *tok); @@ -301,7 +302,7 @@ private: c.checkCastIntToCharAndBackError(nullptr, "func_name"); c.cstyleCastError(nullptr); c.passedByValueError(nullptr, "parametername", false); - c.constVariableError(nullptr); + c.constVariableError(nullptr, nullptr); c.constStatementError(nullptr, "type", false); c.signedCharArrayIndexError(nullptr); c.unknownSignCharArrayIndexError(nullptr); diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 7d9f181f7..660974252 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1053,10 +1053,26 @@ void SymbolDatabase::createSymbolDatabaseSetFunctionPointers(bool firstPass) // Set function call pointers for (const Token* tok = mTokenizer->list.front(); tok != mTokenizer->list.back(); tok = tok->next()) { - if (!tok->function() && tok->varId() == 0 && tok->linkAt(1) && Token::Match(tok, "%name% (") && !isReservedName(tok->str())) { + if (tok->isName() && !tok->function() && tok->varId() == 0 && Token::Match(tok, "%name% [(,)>]") && !isReservedName(tok->str())) { + if (tok->next()->str() == ">" && !tok->next()->link()) + continue; + + if (tok->next()->str() != "(") { + const Token *start = tok; + while (Token::Match(start->tokAt(-2), "%name% ::")) + start = start->tokAt(-2); + if (!Token::Match(start->previous(), "[(,<]") && !Token::Match(start->tokAt(-2), "[(,<] &")) + continue; + } + const Function *function = findFunction(tok); - if (function) - const_cast(tok)->function(function); + if (!function) + continue; + + const_cast(tok)->function(function); + + if (tok->next()->str() != "(") + const_cast(function)->functionPointerUsage = tok; } } @@ -2063,6 +2079,7 @@ Function::Function(const Tokenizer *mTokenizer, noexceptArg(nullptr), throwArg(nullptr), templateDef(nullptr), + functionPointerUsage(nullptr), mFlags(0) { // operator function @@ -2211,6 +2228,7 @@ Function::Function(const Token *tokenDef) noexceptArg(nullptr), throwArg(nullptr), templateDef(nullptr), + functionPointerUsage(nullptr), mFlags(0) { } @@ -4602,10 +4620,7 @@ static std::string getTypeString(const Token *typeToken) const Function* Scope::findFunction(const Token *tok, bool requireConst) const { - // make sure this is a function call - const Token *end = tok->linkAt(1); - if (!end) - return nullptr; + const bool isCall = Token::Match(tok->next(), "(|{"); const std::vector arguments = getArguments(tok); @@ -4617,7 +4632,7 @@ const Function* Scope::findFunction(const Token *tok, bool requireConst) const auto addMatchingFunctions = [&](const Scope *scope) { for (std::multimap::const_iterator it = scope->functionMap.find(tok->str()); it != scope->functionMap.cend() && it->first == tok->str(); ++it) { const Function *func = it->second; - if (args == func->argCount() || + if (!isCall || args == func->argCount() || (func->isVariadic() && args >= (func->argCount() - 1)) || (args < func->argCount() && args >= func->minArgCount())) { matches.push_back(func); @@ -4636,6 +4651,11 @@ const Function* Scope::findFunction(const Token *tok, bool requireConst) const // check in base classes findFunctionInBase(tok->str(), args, matches); + // Non-call => Do not match parameters + if (!isCall) { + return matches.empty() ? nullptr : matches[0]; + } + const Function* fallback1Func = nullptr; const Function* fallback2Func = nullptr; @@ -4862,8 +4882,8 @@ const Function* SymbolDatabase::findFunction(const Token *tok) const } if (currScope) { - while (currScope && !(Token::Match(tok1, "%type% :: %any% (") || - (Token::Match(tok1, "%type% <") && Token::Match(tok1->linkAt(1), "> :: %any% (")))) { + while (currScope && !(Token::Match(tok1, "%type% :: %name% [(),>]") || + (Token::Match(tok1, "%type% <") && Token::Match(tok1->linkAt(1), "> :: %name% (")))) { if (tok1->strAt(1) == "::") tok1 = tok1->tokAt(2); else diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index 6967cac83..d54164681 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -900,6 +900,7 @@ public: const Token *noexceptArg; ///< noexcept token const Token *throwArg; ///< throw token const Token *templateDef; ///< points to 'template <' before function + const Token *functionPointerUsage; ///< function pointer usage static bool argsMatch(const Scope *scope, const Token *first, const Token *second, const std::string &path, nonneg int path_length); diff --git a/test/testother.cpp b/test/testother.cpp index 865f515f4..218e41909 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -98,6 +98,7 @@ private: TEST_CASE(passedByValue_externC); TEST_CASE(constVariable); + TEST_CASE(constParameterCallback); TEST_CASE(switchRedundantAssignmentTest); TEST_CASE(switchRedundantOperationTest); @@ -2522,6 +2523,30 @@ private: TODO_ASSERT_EQUALS("[test.cpp:16]: (style) Parameter 'i' can be declared with const\n", "", errout.str()); } + void constParameterCallback() { + check("int callback(std::vector& x) { return x[0]; }\n" + "void f() { dostuff(callback); }"); + 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()); + + // #9906 + check("class EventEngine : public IEventEngine {\n" + "public:\n" + " EventEngine();\n" + "\n" + "private:\n" + " void signalEvent(ev::sig& signal, int revents);\n" + "};\n" + "\n" + "EventEngine::EventEngine() {\n" + " mSigWatcher.set(this);\n" + "}\n" + "\n" + "void EventEngine::signalEvent(ev::sig& signal, int revents) {\n" + " switch (signal.signum) {}\n" + "}"); + 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()); + } + void switchRedundantAssignmentTest() { check("void foo()\n" "{\n"