From 139071d88b22e2e602f940aa75477311f68bd7c1 Mon Sep 17 00:00:00 2001 From: Robert Reif Date: Thu, 5 Jan 2017 08:52:11 +0100 Subject: [PATCH] Fixed #7875 (New check: function declaration and definition argument names don't match) --- lib/checkother.cpp | 121 +++++++++++++++++++++++++++++++++++++++++++++ lib/checkother.h | 14 +++++- test/testother.cpp | 54 ++++++++++++++++++++ 3 files changed, 188 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 37b9700f2..ed332119c 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2775,3 +2775,124 @@ void CheckOther::accessMovedError(const Token *tok, const std::string &varname, reportError(tok, Severity::warning, errorId, errmsg, CWE672, inconclusive); } + + +void CheckOther::checkFuncArgNamesDifferent() +{ + const bool style = _settings->isEnabled("style"); + const bool inconclusive = _settings->inconclusive; + const bool warning = _settings->isEnabled("warning"); + + if (!(warning || (style && inconclusive))) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + // check every function + for (std::size_t i = 0, end = symbolDatabase->functionScopes.size(); i < end; ++i) { + const Function * function = symbolDatabase->functionScopes[i]->function; + // only check functions with arguments + if (!function || function->argCount() == 0) + continue; + + // only check functions with seperate declarations and definitions + if (function->argDef == function->arg) + continue; + + // get the function argument name tokens + std::vector declarations(function->argCount()); + std::vector definitions(function->argCount()); + const Token * decl = function->argDef->next(); + for (std::size_t j = 0; j < function->argCount(); ++j) { + declarations[j] = nullptr; + definitions[j] = nullptr; + // get the definition + const Variable * variable = function->getArgumentVar(j); + if (variable) { + definitions[j] = variable->nameToken(); + } + // get the declaration (search for first token with varId) + bool skip = false; + while (decl && !Token::Match(decl, ",|)|;")) { + // skip everything after the assignment because + // it could also have a varId or be the first + // token with a varId if there is no name token + if (decl->str() == "=") + skip = true; + // skip over template + else if (decl->link()) + decl = decl->link(); + else if (!skip && decl->varId()) { + declarations[j] = decl; + } + decl = decl->next(); + } + if (decl) + decl = decl->next(); + } + // check for different argument order + if (warning) { + bool order_different = false; + for (std::size_t j = 0; j < function->argCount(); ++j) { + if (!declarations[j] || !definitions[j] || declarations[j]->str() == definitions[j]->str()) + continue; + + for (std::size_t k = 0; k < function->argCount(); ++k) { + if (j != k && definitions[k] && declarations[j]->str() == definitions[k]->str()) { + order_different = true; + break; + } + } + } + if (order_different) { + funcArgOrderDifferent(function->name(), function->argDef->next(), function->arg->next(), declarations, definitions); + continue; + } + } + // check for different argument names + if (style && inconclusive) { + for (std::size_t j = 0; j < function->argCount(); ++j) { + if (declarations[j] && definitions[j] && declarations[j]->str() != definitions[j]->str()) + funcArgNamesDifferent(function->name(), j, declarations[j], definitions[j]); + } + } + } +} + +void CheckOther::funcArgNamesDifferent(const std::string & name, size_t index, + const Token* declaration, const Token* definition) +{ + std::list tokens; + tokens.push_back(declaration); + tokens.push_back(definition); + reportError(tokens, Severity::style, "funcArgNamesDifferent", + "Function '" + name + "' argument " + MathLib::toString(index + 1) + " names different: declaration '" + + (declaration ? declaration->str() : std::string("A")) + "' definition '" + + (definition ? definition->str() : std::string("B")) + "'.", CWE(0U), true); +} + +void CheckOther::funcArgOrderDifferent(const std::string & name, + const Token* declaration, const Token* definition, + const std::vector & declarations, + const std::vector & definitions) +{ + std::list tokens; + tokens.push_back(declarations.size() ? declarations[0] ? declarations[0] : declaration : nullptr); + tokens.push_back(definitions.size() ? definitions[0] ? definitions[0] : definition : nullptr); + std::string msg = "Function '" + name + "' argument order different: declaration '"; + for (std::size_t i = 0; i < declarations.size(); ++i) { + if (i != 0) + msg += ", "; + if (declarations[i]) + msg += declarations[i]->str(); + } + msg += "' definition '"; + for (std::size_t i = 0; i < definitions.size(); ++i) { + if (i != 0) + msg += ", "; + if (definitions[i]) + msg += definitions[i]->str(); + } + msg += "'"; + reportError(tokens, Severity::warning, "funcArgOrderDifferent", msg, CWE(0U), false); +} + diff --git a/lib/checkother.h b/lib/checkother.h index 5a624630c..6033ba954 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -72,6 +72,7 @@ public: checkOther.checkInterlockedDecrement(); checkOther.checkUnusedLabel(); checkOther.checkEvaluationOrder(); + checkOther.checkFuncArgNamesDifferent(); } /** @brief Run checks against the simplified token list */ @@ -207,6 +208,9 @@ public: /** @brief %Check for access of moved or forwarded variable */ void checkAccessOfMovedVariable(); + /** @brief %Check if function declaration and definition argument names different */ + void checkFuncArgNamesDifferent(); + private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result); @@ -258,6 +262,8 @@ private: void unknownEvaluationOrder(const Token* tok); static bool isMovedParameterAllowedForInconclusiveFunction(const Token * tok); void accessMovedError(const Token *tok, const std::string &varname, ValueFlow::Value::MoveKind moveKind, bool inconclusive); + void funcArgNamesDifferent(const std::string & name, size_t index, const Token* declaration, const Token* definition); + void funcArgOrderDifferent(const std::string & name, const Token * declaration, const Token * definition, const std::vector & declarations, const std::vector & definitions); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckOther c(nullptr, settings, errorLogger); @@ -317,6 +323,10 @@ private: c.unknownEvaluationOrder(nullptr); c.accessMovedError(nullptr, "v", ValueFlow::Value::MovedVariable, false); c.accessMovedError(nullptr, "v", ValueFlow::Value::ForwardedVariable, false); + c.funcArgNamesDifferent("function", 1, nullptr, nullptr); + + std::vector nullvec; + c.funcArgOrderDifferent("function", nullptr, nullptr, nullvec, nullvec); } static std::string myName() { @@ -375,7 +385,9 @@ private: "- prefer erfc, expm1 or log1p to avoid loss of precision.\n" "- identical code in both branches of if/else or ternary operator.\n" "- redundant pointer operation on pointer like &*some_ptr.\n" - "- find unused 'goto' labels.\n"; + "- find unused 'goto' labels.\n" + "- function declaration and definition argument names different.\n" + "- function declaration and definition argument order different.\n"; } }; /// @} diff --git a/test/testother.cpp b/test/testother.cpp index 1fbca242a..f70097951 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -192,6 +192,9 @@ private: TEST_CASE(partiallyMoved); TEST_CASE(moveAndLambda); TEST_CASE(forwardAndUsed); + + TEST_CASE(funcArgNamesDifferent); + TEST_CASE(funcArgOrderDifferent); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { @@ -6349,6 +6352,57 @@ private: "}"); ASSERT_EQUALS("[test.cpp:4]: (warning) Access of forwarded variable t.\n", errout.str()); } + + void funcArgNamesDifferent() { + check("void func1(int a, int b, int c); \n" + "void func1(int a, int b, int c) { }\n" + "void func2(int a, int b, int c);\n" + "void func2(int A, int B, int C) { }\n" + "class Fred {\n" + " void func1(int a, int b, int c); \n" + " void func2(int a, int b, int c);\n" + " void func3(int a = 0, int b = 0, int c = 0);\n" + " void func4(int a = 0, int b = 0, int c = 0);\n" + "};\n" + "void Fred::func1(int a, int b, int c) { }\n" + "void Fred::func2(int A, int B, int C) { }\n" + "void Fred::func3(int a, int b, int c) { }\n" + "void Fred::func4(int A, int B, int C) { }\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style, inconclusive) Function 'func2' argument 1 names different: declaration 'a' definition 'A'.\n" + "[test.cpp:3] -> [test.cpp:4]: (style, inconclusive) Function 'func2' argument 2 names different: declaration 'b' definition 'B'.\n" + "[test.cpp:3] -> [test.cpp:4]: (style, inconclusive) Function 'func2' argument 3 names different: declaration 'c' definition 'C'.\n" + "[test.cpp:7] -> [test.cpp:12]: (style, inconclusive) Function 'func2' argument 1 names different: declaration 'a' definition 'A'.\n" + "[test.cpp:7] -> [test.cpp:12]: (style, inconclusive) Function 'func2' argument 2 names different: declaration 'b' definition 'B'.\n" + "[test.cpp:7] -> [test.cpp:12]: (style, inconclusive) Function 'func2' argument 3 names different: declaration 'c' definition 'C'.\n" + "[test.cpp:9] -> [test.cpp:14]: (style, inconclusive) Function 'func4' argument 1 names different: declaration 'a' definition 'A'.\n" + "[test.cpp:9] -> [test.cpp:14]: (style, inconclusive) Function 'func4' argument 2 names different: declaration 'b' definition 'B'.\n" + "[test.cpp:9] -> [test.cpp:14]: (style, inconclusive) Function 'func4' argument 3 names different: declaration 'c' definition 'C'.\n", errout.str()); + } + + void funcArgOrderDifferent() { + check("void func1(int a, int b, int c);\n" + "void func1(int a, int b, int c) { }\n" + "void func2(int a, int b, int c);\n" + "void func2(int c, int b, int a) { }\n" + "void func3(int, int b, int c);\n" + "void func3(int c, int b, int a) { }\n" + "class Fred {\n" + " void func1(int a, int b, int c);\n" + " void func2(int a, int b, int c);\n" + " void func3(int a = 0, int b = 0, int c = 0);\n" + " void func4(int, int b = 0, int c = 0);\n" + "};\n" + "void Fred::func1(int a, int b, int c) { }\n" + "void Fred::func2(int c, int b, int a) { }\n" + "void Fred::func3(int c, int b, int a) { }\n" + "void Fred::func4(int c, int b, int a) { }\n", + nullptr, false, false); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (warning) Function 'func2' argument order different: declaration 'a, b, c' definition 'c, b, a'\n" + "[test.cpp:5] -> [test.cpp:6]: (warning) Function 'func3' argument order different: declaration ', b, c' definition 'c, b, a'\n" + "[test.cpp:9] -> [test.cpp:14]: (warning) Function 'func2' argument order different: declaration 'a, b, c' definition 'c, b, a'\n" + "[test.cpp:10] -> [test.cpp:15]: (warning) Function 'func3' argument order different: declaration 'a, b, c' definition 'c, b, a'\n" + "[test.cpp:11] -> [test.cpp:16]: (warning) Function 'func4' argument order different: declaration ', b, c' definition 'c, b, a'\n", errout.str()); + } }; REGISTER_TEST(TestOther)