From bc409776e3b9ba269e9c4f1894da8c1234f52187 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Tue, 2 Aug 2022 18:31:02 +0200 Subject: [PATCH] Fix #10320 Wrong redundant condition: misleading/wrong message (#4334) * Fix #10320 Wrong redundant condition: misleading/wrong message * Use expressionString() * Clarify condition * Update testcondition.cpp * Trigger CI --- lib/checkcondition.cpp | 81 ++++++++++++++++++++++++++++++++++-------- test/testcondition.cpp | 49 ++++++++++++++----------- 2 files changed, 95 insertions(+), 35 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index fcb9f88ed..fba6db5a0 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -893,6 +893,61 @@ static std::string invertOperatorForOperandSwap(std::string s) return s; } +template +static int sign(const T v) { + return static_cast(v > 0) - static_cast(v < 0); +} + +// returns 1 (-1) if the first (second) condition is sufficient, 0 if indeterminate +template +static int sufficientCondition(std::string op1, const bool not1, const T value1, std::string op2, const bool not2, const T value2, const bool isAnd) { + auto transformOp = [](std::string& op, const bool invert) { + if (invert) { + if (op == "==") + op = "!="; + else if (op == "!=") + op = "=="; + else if (op == "<") + op = ">="; + else if (op == ">") + op = "<="; + else if (op == "<=") + op = ">"; + else if (op == ">=") + op = "<"; + } + }; + transformOp(op1, not1); + transformOp(op2, not2); + int res = 0; + bool equal = false; + if (op1 == op2) { + equal = true; + if (op1 == ">" || op1 == ">=") + res = sign(value1 - value2); + else if (op1 == "<" || op1 == "<=") + res = -sign(value1 - value2); + } else { // not equal + if (op1 == "!=") + res = 1; + else if (op2 == "!=") + res = -1; + else if (op1 == "==") + res = -1; + else if (op2 == "==") + res = 1; + else if (op1 == ">" && op2 == ">=") + res = sign(value1 - (value2 - 1)); + else if (op1 == ">=" && op2 == ">") + res = sign((value1 - 1) - value2); + else if (op1 == "<" && op2 == "<=") + res = -sign(value1 - (value2 + 1)); + else if (op1 == "<=" && op2 == "<") + res = -sign((value1 + 1) - value2); + } + return res * (isAnd == equal ? 1 : -1); +} + template static bool checkIntRelation(const std::string &op, const T value1, const T value2) { @@ -1006,16 +1061,14 @@ static bool parseComparison(const Token *comp, bool *not1, std::string *op, std: static std::string conditionString(bool not1, const Token *expr1, const std::string &op, const std::string &value1) { if (expr1->astParent()->isComparisonOp()) - return std::string(not1 ? "!(" : "") + - (expr1->isName() ? expr1->str() : std::string("EXPR")) + + return std::string(not1 ? "!(" : "") + expr1->expressionString() + " " + op + " " + value1 + (not1 ? ")" : ""); - return std::string(not1 ? "!" : "") + - (expr1->isName() ? expr1->str() : std::string("EXPR")); + return std::string(not1 ? "!" : "") + expr1->expressionString(); } static std::string conditionString(const Token * tok) @@ -1198,6 +1251,7 @@ void CheckCondition::checkIncorrectLogicOperator() // evaluate if expression is always true/false bool alwaysTrue = true, alwaysFalse = true; bool firstTrue = true, secondTrue = true; + const bool isAnd = tok->str() == "&&"; for (int test = 1; test <= 5; ++test) { // test: // 1 => testvalue is less than both value1 and value2 @@ -1223,7 +1277,7 @@ void CheckCondition::checkIncorrectLogicOperator() result1 = !result1; if (not2) result2 = !result2; - if (tok->str() == "&&") { + if (isAnd) { alwaysTrue &= (result1 && result2); alwaysFalse &= !(result1 && result2); } else { @@ -1239,16 +1293,13 @@ void CheckCondition::checkIncorrectLogicOperator() if (printWarning && (alwaysTrue || alwaysFalse)) { const std::string text = cond1str + " " + tok->str() + " " + cond2str; incorrectLogicOperatorError(tok, text, alwaysTrue, inconclusive, errorPath); - } else if (printStyle && secondTrue) { - const std::string text = "If '" + cond1str + "', the comparison '" + cond2str + - "' is always true."; - redundantConditionError(tok, text, inconclusive); - } else if (printStyle && firstTrue) { - //const std::string text = "The comparison " + cond1str + " is always " + - // (firstTrue ? "true" : "false") + " when " + - // cond2str + "."; - const std::string text = "If '" + cond2str + "', the comparison '" + cond1str + - "' is always true."; + } else if (printStyle && (firstTrue || secondTrue)) { + const int which = sufficientCondition(op1, not1, i1, op2, not2, i2, isAnd); + std::string text; + if (which != 0) { + text = "The condition '" + (which == 1 ? cond2str : cond1str) + "' is redundant since '" + (which == 1 ? cond1str : cond2str) + "' is sufficient."; + } else + text = "If '" + (secondTrue ? cond1str : cond2str) + "', the comparison '" + (secondTrue ? cond2str : cond1str) + "' is always true."; redundantConditionError(tok, text, inconclusive); } } diff --git a/test/testcondition.cpp b/test/testcondition.cpp index ecbbfb835..f2e6d4ca6 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -1162,7 +1162,7 @@ private: " if((x==3) && (x!=4))\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x != 4' is redundant since 'x == 3' is sufficient.\n", errout.str()); check("void f(const std::string &s) {\n" // #8860 " const std::size_t p = s.find(\"42\");\n" @@ -1175,19 +1175,19 @@ private: " if ((x!=4) && (x==3))\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x != 4' is redundant since 'x == 3' is sufficient.\n", errout.str()); check("void f(int x) {\n" " if ((x==3) || (x!=4))\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x == 3' is redundant since 'x != 4' is sufficient.\n", errout.str()); check("void f(int x) {\n" " if ((x!=4) || (x==3))\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x == 3' is redundant since 'x != 4' is sufficient.\n", errout.str()); check("void f(int x) {\n" " if ((x==3) && (x!=3))\n" @@ -1217,7 +1217,7 @@ private: " if (x > 5 && x == 6)\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 6', the comparison 'x > 5' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x > 5' is redundant since 'x == 6' is sufficient.\n", errout.str()); // #3419 check("void f() {\n" @@ -1281,7 +1281,7 @@ private: check("void f(int x) {\n" " if (x+3 > 2 || x+3 < 10) {}\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: EXPR > 2 || EXPR < 10.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x+3 > 2 || x+3 < 10.\n", errout.str()); } void incorrectLogicOperator6() { // char literals @@ -1691,7 +1691,7 @@ private: " if (x > 5 && x != 1)\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x > 5', the comparison 'x != 1' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x != 1' is redundant since 'x > 5' is sufficient.\n", errout.str()); check("void f(int x) {\n" " if (x > 5 && x != 6)\n" @@ -1703,7 +1703,7 @@ private: " if ((x > 5) && (x != 1))\n" " a++;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x > 5', the comparison 'x != 1' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x != 1' is redundant since 'x > 5' is sufficient.\n", errout.str()); check("void f(int x) {\n" " if ((x > 5) && (x != 6))\n" @@ -1717,10 +1717,11 @@ private: " d = x >= 3 || x == 4;\n" " e = x <= 5 || x == 4;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 4', the comparison 'x > 3' is always true.\n" - "[test.cpp:3]: (style) Redundant condition: If 'x == 4', the comparison 'x < 5' is always true.\n" - "[test.cpp:4]: (style) Redundant condition: If 'x == 4', the comparison 'x >= 3' is always true.\n" - "[test.cpp:5]: (style) Redundant condition: If 'x == 4', the comparison 'x <= 5' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x == 4' is redundant since 'x > 3' is sufficient.\n" + "[test.cpp:3]: (style) Redundant condition: The condition 'x == 4' is redundant since 'x < 5' is sufficient.\n" + "[test.cpp:4]: (style) Redundant condition: The condition 'x == 4' is redundant since 'x >= 3' is sufficient.\n" + "[test.cpp:5]: (style) Redundant condition: The condition 'x == 4' is redundant since 'x <= 5' is sufficient.\n", + errout.str()); check("void f(int x, bool& b) {\n" " b = x > 5 || x != 1;\n" @@ -1728,10 +1729,11 @@ private: " d = x >= 5 || x != 1;\n" " e = x <= 1 || x != 3;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x > 5', the comparison 'x != 1' is always true.\n" - "[test.cpp:3]: (style) Redundant condition: If 'x < 1', the comparison 'x != 3' is always true.\n" - "[test.cpp:4]: (style) Redundant condition: If 'x >= 5', the comparison 'x != 1' is always true.\n" - "[test.cpp:5]: (style) Redundant condition: If 'x <= 1', the comparison 'x != 3' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x > 5' is redundant since 'x != 1' is sufficient.\n" + "[test.cpp:3]: (style) Redundant condition: The condition 'x < 1' is redundant since 'x != 3' is sufficient.\n" + "[test.cpp:4]: (style) Redundant condition: The condition 'x >= 5' is redundant since 'x != 1' is sufficient.\n" + "[test.cpp:5]: (style) Redundant condition: The condition 'x <= 1' is redundant since 'x != 3' is sufficient.\n", + errout.str()); check("void f(int x, bool& b) {\n" " b = x > 6 && x > 5;\n" @@ -1739,10 +1741,17 @@ private: " d = x < 6 && x < 5;\n" " e = x < 5 || x < 6;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x > 6', the comparison 'x > 5' is always true.\n" - "[test.cpp:3]: (style) Redundant condition: If 'x > 6', the comparison 'x > 5' is always true.\n" - "[test.cpp:4]: (style) Redundant condition: If 'x < 5', the comparison 'x < 6' is always true.\n" - "[test.cpp:5]: (style) Redundant condition: If 'x < 5', the comparison 'x < 6' is always true.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x > 5' is redundant since 'x > 6' is sufficient.\n" + "[test.cpp:3]: (style) Redundant condition: The condition 'x > 6' is redundant since 'x > 5' is sufficient.\n" + "[test.cpp:4]: (style) Redundant condition: The condition 'x < 6' is redundant since 'x < 5' is sufficient.\n" + "[test.cpp:5]: (style) Redundant condition: The condition 'x < 5' is redundant since 'x < 6' is sufficient.\n", + errout.str()); + + check("void f(const char *p) {\n" // #10320 + " if (!p || !*p || *p != 'x') {}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition '!*p' is redundant since '*p != 'x'' is sufficient.\n", + errout.str()); } void incorrectLogicOp_condSwapping() {