Fixed #7875 (New check: function declaration and definition argument names don't match)

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<const Token *>  declarations(function->argCount());
+        std::vector<const Token *>  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<const Token *> 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<const Token *> & declarations,
+                                       const std::vector<const Token *> & definitions)
+{
+    std::list<const Token *> 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);
+}
+

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<const Token*> & declarations, const std::vector<const Token*> & 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<const Token *> 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";
     }
 };
 /// @}

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)