fix #5591: Add check for meaningless comma operator in if statement, misplaced ')'

lib/checkother.cpp

@@ -4502,6 +4502,43 @@ void CheckOther::overlappingWriteFunction(const Token *tok, const std::string& f
     reportError(tok, Severity::error, "overlappingWriteFunction", "Overlapping read/write in " + funcname + "() is undefined behavior");
 }
 
+void CheckOther::checkSuspiciousComma()
+{
+    if (!mSettings->severity.isEnabled(Severity::style)) {
+        return;
+    }
+
+    logChecker("CheckOther::suspiciousComma");
+
+    for (const Token* tok = mTokenizer->list.front(); tok; tok = tok->next()) {
+        if (tok->str() == "," && tok->isBinaryOp()) {
+            const Token * parent = tok->astParent();
+            if (parent && Token::Match(parent->previous(), "if|while (")) {
+                if (tok->strAt(-1) == ")") {
+                    const Function * func = tok->linkAt(-1)->previous()->function();
+                    if (func && func->initArgCount > 0 && !tok->astOperand2()->hasKnownValue()) {
+                        std::vector<const Token*> arg_vec = getArguments(tok->linkAt(-1)->previous());
+                        if (arg_vec.size() < func->argCount() && !arg_vec.empty()) {
+                            const ValueType * type1 = tok->astOperand2()->valueType();
+                            std::vector<Variable> v(func->argumentList.begin(), func->argumentList.end());
+                            const ValueType * type2 = v[arg_vec.size()].valueType();
+                            if (type1 && type2 && type1->isTypeEqual(type2)) {
+                                checkSuspiciousCommaError(tok);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+    }
+}
+
+void CheckOther::checkSuspiciousCommaError(const Token *tok)
+{
+    reportError(tok, Severity::style, "suspiciousComma", "There is a suspicious comma expression directly after a function call "
+                "in an if/while condition statement, is there a misplaced paranthesis?");
+}
+
 void CheckOther::runChecks(const Tokenizer &tokenizer, ErrorLogger *errorLogger)
 {
     CheckOther checkOther(&tokenizer, &tokenizer.getSettings(), errorLogger);
@@ -4551,6 +4588,7 @@ void CheckOther::runChecks(const Tokenizer &tokenizer, ErrorLogger *errorLogger)
     checkOther.checkAccessOfMovedVariable();
     checkOther.checkModuloOfOne();
     checkOther.checkOverlappingWrite();
+    checkOther.checkSuspiciousComma();
 }
 
 void CheckOther::getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const
@@ -4625,6 +4663,7 @@ void CheckOther::getErrorMessages(ErrorLogger *errorLogger, const Settings *sett
     c.comparePointersError(nullptr, nullptr, nullptr);
     c.redundantAssignmentError(nullptr, nullptr, "var", false);
     c.redundantInitializationError(nullptr, nullptr, "var", false);
+    c.checkSuspiciousCommaError(nullptr);
 
     const std::vector<const Token *> nullvec;
     c.funcArgOrderDifferent("function", nullptr, nullptr, nullvec, nullvec);

lib/checkother.h

@@ -198,6 +198,8 @@ class CPPCHECKLIB CheckOther : public Check {
     void overlappingWriteUnion(const Token *tok);
     void overlappingWriteFunction(const Token *tok, const std::string& funcname);
 
+    void checkSuspiciousComma();
+
     // Error messages..
     void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &functionName, const std::string &varName, bool result);
     void checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName);
@@ -257,6 +259,7 @@ class CPPCHECKLIB CheckOther : public Check {
     void knownPointerToBoolError(const Token* tok, const ValueFlow::Value* value);
     void comparePointersError(const Token *tok, const ValueFlow::Value *v1, const ValueFlow::Value *v2);
     void checkModuloOfOneError(const Token *tok);
+    void checkSuspiciousCommaError(const Token *tok);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override;
 
@@ -322,7 +325,8 @@ class CPPCHECKLIB CheckOther : public Check {
                "- shadow variable.\n"
                "- variable can be declared const.\n"
                "- calculating modulo of one.\n"
-               "- known function argument, suspicious calculation.\n";
+               "- known function argument, suspicious calculation.\n"
+               "- suspicious comma immediately after function call in if/while condition statement.\n";
     }
 
     bool diag(const Token* tok) {

man/checkers/suspiciousComma.md

@@ -0,0 +1,25 @@
+
+# suspiciousComma
+
+**Message**: There is a suspicious comma expression directly after a function call in an if/while condition statement, is there a misplaced paranthesis?<br/>
+**Category**: ImproveCheck<br/>
+**Severity**: Style<br/>
+**Language**: C and C++
+
+## Description
+
+When calling functions in an if or while condition statement, if the conditon is complex containing many parentheses, sometimes the parenthese might be misplaced if not carefull enough. If the function has a default argument, the compiler will not report errors, and the error may not be found and cause some unexpected result while the program running.
+```
+int func(int a, int b = 100) {
+    return a+b;
+}
+
+int call(int var) {
+    if(func(func(100, 100)), var) {} //what actually wanted might be func(func(100, 100), var).
+}
+```
+This is just to detect a most likely error, which could be some code wrote as intended with a bad style but not a real coding error.
+
+## How to fix
+
+The fix is simple. Just check the code carefully and make sure the function calls are as intended.

releasenotes.txt

@@ -4,6 +4,7 @@ New checks:
 -
 
 Improved checking:
+- Check the suspicious comma expression directly after a function call in an if/while condition statement.
 -
 
 GUI:

test/testother.cpp

@@ -307,6 +307,8 @@ class TestOther : public TestFixture {
 
         TEST_CASE(knownConditionFloating);
         TEST_CASE(knownConditionPrefixed);
+
+        TEST_CASE(suspiciousComma);
     }
 
 #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__)
@@ -13195,6 +13197,43 @@ class TestOther : public TestFixture {
             "[test.cpp:2:13] -> [test.cpp:3:11]: (style) The comparison 'i > +1' is always false. [knownConditionTrueFalse]\n",
             errout_str());
     }
+
+    void suspiciousComma()
+    {
+        check("void f(int a, int b = 0);\n"
+              "void foo(int var) {\n"
+              "    if (f(100), var) {}\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3:15]: (style) There is a suspicious comma expression directly after a function call "
+            "in an if/while condition statement, is there a misplaced paranthesis? [suspiciousComma]\n",
+            errout_str());
+
+        check("void f(int a, int b = 0);\n"
+              "void foo(int var) {\n"
+              "    while (f(100), var) {}\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "[test.cpp:3:18]: (style) There is a suspicious comma expression directly after a function call "
+            "in an if/while condition statement, is there a misplaced paranthesis? [suspiciousComma]\n",
+            errout_str());
+
+        check("void f(int a, int b = 0);\n"
+              "void foo(int * var) {\n"
+              "    if (f(100), var) {}\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "",
+            errout_str());
+
+        check("void f(int a);\n"
+              "void foo(int var) {\n"
+              "    if (f(100), var) {}\n"
+              "}\n");
+        ASSERT_EQUALS(
+            "",
+            errout_str());
+    }
 };
 
 REGISTER_TEST(TestOther)