diff --git a/lib/checkother.cpp b/lib/checkother.cpp index ae210f7d8..d02c9ec80 100644 --- a/lib/checkother.cpp +++ b/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 //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index c1af985c4..5707c8acb 100644 --- a/lib/checkother.h +++ b/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 &constFunctions, diff --git a/test/testother.cpp b/test/testother.cpp index 845b12fa2..6c67e2668 100644 --- a/test/testother.cpp +++ b/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)