From bce5fe5cefedbfa020e236a0fa8c6d6e55ccc8ef Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Tue, 3 Apr 2018 14:43:55 -0500 Subject: [PATCH] Improve duplicate expressions in the ternary op by checking for equal values as well (#1134) * Improve duplicate expressions in the ternary op by checking for equal values as well * Use value instead of expression --- lib/checkother.cpp | 11 ++++++++++- lib/checkother.h | 2 ++ test/testother.cpp | 37 ++++++++++++++++++++++++++++++++----- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 0bdac8cb0..77623a58b 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1973,7 +1973,9 @@ void CheckOther::checkDuplicateExpression() } } } else if (styleEnabled && tok->astOperand1() && tok->astOperand2() && tok->str() == ":" && tok->astParent() && tok->astParent()->str() == "?") { - if (isSameExpression(_tokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), _settings->library, false)) + if (!tok->astOperand1()->values().empty() && !tok->astOperand2()->values().empty() && isEqualKnownValue(tok->astOperand1(), tok->astOperand2())) + duplicateValueTernaryError(tok); + else if (isSameExpression(_tokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), _settings->library, false)) duplicateExpressionTernaryError(tok); } } @@ -1997,6 +1999,13 @@ void CheckOther::duplicateExpressionTernaryError(const Token *tok) "the same code is executed regardless of the condition.", CWE398, false); } +void CheckOther::duplicateValueTernaryError(const Token *tok) +{ + reportError(tok, Severity::style, "duplicateValueTernary", "Same value in both branches of ternary operator.\n" + "Finding the same value in both branches of ternary operator is suspicious as " + "the same code is executed regardless of the condition.", CWE398, false); +} + void CheckOther::selfAssignmentError(const Token *tok, const std::string &varname) { reportError(tok, Severity::warning, diff --git a/lib/checkother.h b/lib/checkother.h index b17601bb1..b3b4b0467 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -239,6 +239,7 @@ private: void misusedScopeObjectError(const Token *tok, const std::string &varname); void duplicateBranchError(const Token *tok1, const Token *tok2); void duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string &op); + void duplicateValueTernaryError(const Token *tok); void duplicateExpressionTernaryError(const Token *tok); void duplicateBreakError(const Token *tok, bool inconclusive); void unreachableCodeError(const Token* tok, bool inconclusive); @@ -297,6 +298,7 @@ private: c.clarifyStatementError(nullptr); c.duplicateBranchError(nullptr, nullptr); c.duplicateExpressionError(nullptr, nullptr, "&&"); + c.duplicateValueTernaryError(nullptr); c.duplicateExpressionTernaryError(nullptr); c.duplicateBreakError(nullptr, false); c.unreachableCodeError(nullptr, false); diff --git a/test/testother.cpp b/test/testother.cpp index 657e93a0d..2ba0aae4d 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -128,6 +128,7 @@ private: TEST_CASE(duplicateExpression4); // ticket #3354 (++) TEST_CASE(duplicateExpression5); // ticket #3749 (macros with same values) TEST_CASE(duplicateExpression6); // ticket #4639 + TEST_CASE(duplicateValueTernary); TEST_CASE(duplicateExpressionTernary); // #6391 TEST_CASE(duplicateExpressionTemplate); // #6930 @@ -3855,11 +3856,6 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (style) Same expression in both branches of ternary operator.\n", errout.str()); - check("void f() {\n" - " if( a ? (b ? false:false): false ) ;\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Same expression in both branches of ternary operator.\n", errout.str()); - check("void f() {\n" " return A ? x : z;\n" "}"); @@ -3876,6 +3872,37 @@ private: ASSERT_EQUALS("", errout.str()); } + void duplicateValueTernary() { + check("void f() {\n" + " if( a ? (b ? false:false): false ) ;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Same value in both branches of ternary operator.\n", errout.str()); + + check("int f1(int a) {return (a == 1) ? (int)1 : 1; }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Same value in both branches of ternary operator.\n", errout.str()); + + check("int f2(int a) {return (a == 1) ? (int)1 : (int)1; }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Same value in both branches of ternary operator.\n", errout.str()); + + check("int f3(int a) {return (a == 1) ? 1 : (int)1; }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Same value in both branches of ternary operator.\n", errout.str()); + + check("int f4(int a) {return (a == 1) ? 1 : 1; }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Same value in both branches of ternary operator.\n", errout.str()); + + check("int f5(int a) {return (a == (int)1) ? (int)1 : 1; }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Same value in both branches of ternary operator.\n", errout.str()); + + check("int f6(int a) {return (a == (int)1) ? (int)1 : (int)1; }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Same value in both branches of ternary operator.\n", errout.str()); + + check("int f7(int a) {return (a == (int)1) ? 1 : (int)1; }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Same value in both branches of ternary operator.\n", errout.str()); + + check("int f8(int a) {return (a == (int)1) ? 1 : 1; }"); + ASSERT_EQUALS("[test.cpp:1]: (style) Same value in both branches of ternary operator.\n", errout.str()); + } + void duplicateExpressionTemplate() { // #6930 check("template void f() {\n" " if (I >= 0 && I < 3) {}\n"