Fixed #4104 (New check: comma separated statements in return statement from a function)

lib/checkother.cpp

@@ -1877,6 +1877,46 @@ void CheckOther::variableScopeError(const Token *tok, const std::string &varname
                 "When you see this message it is always safe to reduce the variable scope 1 level.");
 }
 
+void CheckOther::checkCommaSeparatedReturn()
+{
+    if (!_settings->isEnabled("style"))
+        return;
+
+    for (const Token *tok = _tokenizer->tokens(); tok ; tok = tok->next())      {
+        if (Token::Match(tok ,"return")) {
+
+            while (tok->str() != ";")   {
+
+                if (tok->str() == "(")
+                    tok=tok->link();
+
+                if (!tok->isExpandedMacro() && tok->str() == ",")
+                    commaSeparatedReturnError(tok);
+
+                tok=tok->next();
+
+            }
+        }
+    }
+}
+
+void CheckOther::commaSeparatedReturnError(const Token *tok)
+{
+    reportError(tok,
+                Severity::style,
+                "commaSeparatedReturn",
+                "Comma is used in return statement. The comma can easily be misread as a ';'.\n"
+                "Comma is used in return statement. When comma is used in a return statement it can "
+                "easily be misread as a semicolon. For example in the code below the value "
+                "of 'b' is returned if the condition is true, but it is easy to think that 'a+1' is "
+                "returned:\n"
+                "    if (x)\n"
+                "        return a + 1,\n"
+                "    b++;\n"
+                "However it can be useful to use comma in macros. Cppcheck does not warn when such a "
+                "macro is then used in a return statement, It is less likely such code is misunderstood.");
+}
+
 //---------------------------------------------------------------------------
 // Check for constant function parameters
 //---------------------------------------------------------------------------

lib/checkother.h

@@ -73,6 +73,7 @@ public:
         checkOther.checkSuspiciousStringCompare();
         checkOther.checkVarFuncNullUB();
         checkOther.checkNanInArithmeticExpression();
+        checkOther.checkCommaSeparatedReturn();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -144,6 +145,9 @@ public:
     void checkVariableScope();
     static bool checkInnerScope(const Token *tok, const Variable* var, bool& used);
 
+    /** @brief %Check for comma separated statements in return */
+    void checkCommaSeparatedReturn();
+
     /** @brief %Check for constant function parameter */
     void checkConstantFunctionParameter();
 
@@ -319,6 +323,7 @@ private:
     void redundantCopyError(const Token *tok, const std::string &varname);
     void incompleteArrayFillError(const Token* tok, const std::string& buffer, const std::string& function, bool boolean);
     void varFuncNullUBError(const Token *tok);
+    void commaSeparatedReturnError(const Token *tok);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckOther c(0, settings, errorLogger);
@@ -383,6 +388,7 @@ private:
         c.incompleteArrayFillError(0, "buffer", "memset", false);
         c.varFuncNullUBError(0);
         c.nanInArithmeticExpressionError(0);
+        c.commaSeparatedReturnError(0);
     }
 
     static std::string myName() {
@@ -445,7 +451,8 @@ private:
                "* Array filled incompletely using memset/memcpy/memmove.\n"
                "* redundant get and set function of user id (--std=posix).\n"
                "* Passing NULL pointer to function with variable number of arguments leads to UB on some platforms.\n"
-               "* NaN (not a number) value used in arithmetic expression.\n";
+               "* NaN (not a number) value used in arithmetic expression.\n"
+               "* comma in return statement (the comma can easily be misread as a semicolon).\n";
     }
 
     void checkExpressionRange(const std::list<const Function*> &constFunctions,

test/testother.cpp

@@ -172,6 +172,8 @@ private:
         TEST_CASE(checkCastIntToCharAndBack); // ticket #160
 
         TEST_CASE(checkSleepTimeIntervall)
+
+        TEST_CASE(checkCommaSeparatedReturn);
     }
 
     void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true, bool posix = false, bool runSimpleChecks=true) {
@@ -6232,6 +6234,20 @@ private:
               "}",NULL,false,false,true);
         ASSERT_EQUALS("[test.cpp:2]: (error) The argument of usleep must be less than 1000000.\n", errout.str());
     }
+
+    void checkCommaSeparatedReturn() {
+        check("int fun(int a) {\n"
+              "  if (a < 0)\n"
+              "    return a++ , 0;\n"
+              "}", NULL, false, false, false, false);
+        ASSERT_EQUALS("[test.cpp:3]: (style) Comma is used in return statement. The comma can easily be misread as a ';'.\n", errout.str());
+
+        check("int fun(int a) {\n"
+              "  if (a < 0)\n"
+              "    return a=a+5,1; \n"
+              "}", NULL, false, false, false, false);
+        ASSERT_EQUALS("[test.cpp:3]: (style) Comma is used in return statement. The comma can easily be misread as a ';'.\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)