diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 82fe0911f..d2a9cf7f4 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1556,7 +1556,7 @@ void CheckOther::checkConstPointer() const ValueType* const vt = tok->valueType(); if (!vt) continue; - if (vt->pointer != 1 && !(vt->pointer == 2 && var->isArray()) || (vt->constness & 1) || vt->reference != Reference::None) + if ((vt->pointer != 1 && !(vt->pointer == 2 && var->isArray())) || (vt->constness & 1) || vt->reference != Reference::None) continue; if (std::find(nonConstPointers.begin(), nonConstPointers.end(), var) != nonConstPointers.end()) continue; @@ -2417,13 +2417,17 @@ void CheckOther::checkDuplicateExpression() isWithoutSideEffects(mTokenizer->isCPP(), tok->astOperand2())) duplicateExpressionError(tok->astOperand2(), tok->astOperand1()->astOperand2(), tok, errorPath); else if (tok->astOperand2() && isConstExpression(tok->astOperand1(), mSettings->library, true, mTokenizer->isCPP())) { - const Token *ast1 = tok->astOperand1(); - while (ast1 && tok->str() == ast1->str()) { - if (isSameExpression(mTokenizer->isCPP(), true, ast1->astOperand1(), tok->astOperand2(), mSettings->library, true, true, &errorPath) && - isWithoutSideEffects(mTokenizer->isCPP(), ast1->astOperand1()) && + auto checkDuplicate = [&](const Token* exp1, const Token* exp2, const Token* ast1) { + if (isSameExpression(mTokenizer->isCPP(), true, exp1, exp2, mSettings->library, true, true, &errorPath) && + isWithoutSideEffects(mTokenizer->isCPP(), exp1) && isWithoutSideEffects(mTokenizer->isCPP(), ast1->astOperand2())) - // Probably the message should be changed to 'duplicate expressions X in condition or something like that'. - duplicateExpressionError(ast1->astOperand1(), tok->astOperand2(), tok, errorPath); + duplicateExpressionError(exp1, exp2, tok, errorPath, /*hasMultipleExpr*/ true); + }; + const Token *ast1 = tok->astOperand1(); + while (ast1 && tok->str() == ast1->str()) { // chain of identical operators + checkDuplicate(ast1->astOperand2(), tok->astOperand2(), ast1); + if (ast1->astOperand1() && ast1->astOperand1()->str() != tok->str()) // check first condition in the chain + checkDuplicate(ast1->astOperand1(), tok->astOperand2(), ast1); ast1 = ast1->astOperand1(); } } @@ -2452,7 +2456,7 @@ void CheckOther::oppositeExpressionError(const Token *opTok, ErrorPath errors) "determine if it is correct.", CWE398, Certainty::normal); } -void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2, const Token *opTok, ErrorPath errors) +void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2, const Token *opTok, ErrorPath errors, bool hasMultipleExpr) { errors.emplace_back(opTok, ""); @@ -2460,7 +2464,7 @@ void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string& expr2 = tok2 ? tok2->expressionString() : "x"; const std::string& op = opTok ? opTok->str() : "&&"; - std::string msg = "Same expression on both sides of \'" + op + "\'"; + std::string msg = "Same expression " + (hasMultipleExpr ? "\'" + expr1 + "\'" + " found multiple times in chain of \'" + op + "\' operators" : "on both sides of \'" + op + "\'"); const char *id = "duplicateExpression"; if (expr1 != expr2 && (!opTok || !opTok->isArithmeticalOp())) { id = "knownConditionTrueFalse"; @@ -2473,9 +2477,9 @@ void CheckOther::duplicateExpressionError(const Token *tok1, const Token *tok2, msg += " because '" + expr1 + "' and '" + expr2 + "' represent the same value"; } - reportError(errors, Severity::style, id, msg + ".\n" - "Finding the same expression on both sides of an operator is suspicious and might " - "indicate a cut and paste or logic error. Please examine this code carefully to " + reportError(errors, Severity::style, id, msg + + (std::string(".\nFinding the same expression ") + (hasMultipleExpr ? "more than once in a condition" : "on both sides of an operator")) + + " is suspicious and might indicate a cut and paste or logic error. Please examine this code carefully to " "determine if it is correct.", CWE398, Certainty::normal); } diff --git a/lib/checkother.h b/lib/checkother.h index 8f8403526..6c822e7c6 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -260,7 +260,7 @@ private: void duplicateBranchError(const Token *tok1, const Token *tok2, ErrorPath errors); void duplicateAssignExpressionError(const Token *tok1, const Token *tok2, bool inconclusive); void oppositeExpressionError(const Token *opTok, ErrorPath errors); - void duplicateExpressionError(const Token *tok1, const Token *tok2, const Token *opTok, ErrorPath errors); + void duplicateExpressionError(const Token *tok1, const Token *tok2, const Token *opTok, ErrorPath errors, bool hasMultipleExpr = false); void duplicateValueTernaryError(const Token *tok); void duplicateExpressionTernaryError(const Token *tok, ErrorPath errors); void duplicateBreakError(const Token *tok, bool inconclusive); diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 09c265f64..399859a5e 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -3382,15 +3382,13 @@ private: "[test.cpp:3] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n", errout.str()); - // TODO: Ast is missing for this case check("std::vector f() {\n" " int i = 0;\n" " std::vector v{&i, &i};\n" " return v;\n" "}"); - TODO_ASSERT_EQUALS( + ASSERT_EQUALS( "[test.cpp:3] -> [test.cpp:3] -> [test.cpp:2] -> [test.cpp:4]: (error) Returning object that points to local variable 'i' that will be invalid when returning.\n", - "", errout.str()); check("std::vector f() {\n" diff --git a/test/testother.cpp b/test/testother.cpp index af712faff..c9bb1a14e 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -160,6 +160,7 @@ private: TEST_CASE(duplicateExpression13); // #7899 TEST_CASE(duplicateExpression14); // #9871 TEST_CASE(duplicateExpression15); // #10650 + TEST_CASE(duplicateExpression16); // #10569 TEST_CASE(duplicateExpressionLoop); TEST_CASE(duplicateValueTernary); TEST_CASE(duplicateExpressionTernary); // #6391 @@ -5125,7 +5126,7 @@ private: check("void foo() {\n" " if (x!=2 || y!=3 || x!=2) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Same expression 'x!=2' found multiple times in chain of '||' operators.\n", errout.str()); check("void foo() {\n" " if (x!=2 && (x=y) && x!=2) {}\n" @@ -5601,7 +5602,8 @@ private: " const bool c = a;\n" " return a && b && c;\n" "}"); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression on both sides of '&&' because 'a' and 'c' represent the same value.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Same expression 'a' found multiple times in chain of '&&' operators because 'a' and 'c' represent the same value.\n", + errout.str()); // 6906 check("void f(const bool b) {\n" @@ -5785,6 +5787,31 @@ private: errout.str()); } + void duplicateExpression16() { //#10569 + check("void f(const std::string& a) {\n" + " if ((a == \"x\") ||\n" + " (a == \"42\") ||\n" + " (a == \"y\") ||\n" + " (a == \"42\")) {}\n" + "}\n" + "void g(const std::string& a) {\n" + " if ((a == \"42\") ||\n" + " (a == \"x\") ||\n" + " (a == \"42\") ||\n" + " (a == \"y\")) {}\n" + "}\n" + "void h(const std::string& a) {\n" + " if ((a == \"42\") ||\n" + " (a == \"x\") ||\n" + " (a == \"y\") ||\n" + " (a == \"42\")) {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:4]: (style) Same expression 'a==\"42\"' found multiple times in chain of '||' operators.\n" + "[test.cpp:7] -> [test.cpp:9]: (style) Same expression 'a==\"42\"' found multiple times in chain of '||' operators.\n" + "[test.cpp:13] -> [test.cpp:16]: (style) Same expression 'a==\"42\"' found multiple times in chain of '||' operators.\n", + errout.str()); + } + void duplicateExpressionLoop() { check("void f() {\n" " int a = 1;\n"