From 9895ea5ff21a93c24089fa64f8721236aaba80df Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 23 Jul 2018 01:51:59 -0500 Subject: [PATCH] Fix issue 470: Condition is always true or false on logical operators (#1294) * Fix issue 470: Condition is always true or false on logical operators * Dont warn on literals * Compute logical operators using valueflow * Fix FP when using literals * Always warn on subconditions that are always true * Use percent matches first * Add test for logical operators * Check if parent is null --- lib/checkcondition.cpp | 6 ++--- lib/valueflow.cpp | 12 ++++++++++ test/testcondition.cpp | 31 ++++++++++++++++++++++++++ test/testvalueflow.cpp | 50 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 4 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index b86f2b87f..efbd92604 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -885,7 +885,6 @@ void CheckCondition::checkIncorrectLogicOperator() continue; } - // 'A && (!A || B)' is equivalent to 'A && B' // 'A || (!A && B)' is equivalent to 'A || B' if (printStyle && @@ -1192,9 +1191,8 @@ void CheckCondition::alwaysTrueFalse() continue; const bool constIfWhileExpression = - tok->astParent() - && Token::Match(tok->astParent()->astOperand1(), "if|while") - && !tok->isBoolean(); + tok->astParent() && Token::Match(tok->astTop()->astOperand1(), "if|while") && + (Token::Match(tok->astParent(), "%oror%|&&") || Token::Match(tok->astParent()->astOperand1(), "if|while")); const bool constValExpr = tok->isNumber() && Token::Match(tok->astParent(),"%oror%|&&|?"); // just one number in boolean expression const bool compExpr = Token::Match(tok, "%comp%|!"); // a compare expression diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 194153001..246178f40 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -444,6 +444,18 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti return; } + // known result when a operand is true. + if (Token::simpleMatch(parent, "&&") && value.isKnown() && value.isIntValue() && value.intvalue==0) { + setTokenValue(parent, value, settings); + return; + } + + // known result when a operand is false. + if (Token::simpleMatch(parent, "||") && value.isKnown() && value.isIntValue() && value.intvalue!=0) { + setTokenValue(parent, value, settings); + return; + } + for (const ValueFlow::Value &value1 : parent->astOperand1()->values()) { if (!value1.isIntValue() && !value1.isFloatValue() && !value1.isTokValue()) continue; diff --git a/test/testcondition.cpp b/test/testcondition.cpp index f54ff5a83..a9c89b268 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -105,6 +105,7 @@ private: TEST_CASE(checkInvalidTestForOverflow); TEST_CASE(checkConditionIsAlwaysTrueOrFalseInsideIfWhile); + TEST_CASE(alwaysTrueFalseInLogicalOperators); TEST_CASE(pointerAdditionResultNotNull); } @@ -2507,6 +2508,36 @@ private: ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'a+1' is always true\n", errout.str()); } + void alwaysTrueFalseInLogicalOperators() { + check("bool f();\n" + "void foo() { bool x = true; if(x||f()) {}}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always true\n", errout.str()); + + check("void foo(bool b) { bool x = true; if(x||b) {}}\n"); + ASSERT_EQUALS("[test.cpp:1]: (style) Condition 'x' is always true\n", errout.str()); + + check("void foo(bool b) { if(true||b) {}}\n"); + ASSERT_EQUALS("", errout.str()); + + check("bool f();\n" + "void foo() { bool x = false; if(x||f()) {}}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always false\n", errout.str()); + + check("bool f();\n" + "void foo() { bool x = false; if(x&&f()) {}}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always false\n", errout.str()); + + check("void foo(bool b) { bool x = false; if(x&&b) {}}\n"); + ASSERT_EQUALS("[test.cpp:1]: (style) Condition 'x' is always false\n", errout.str()); + + check("void foo(bool b) { if(false&&b) {}}\n"); + ASSERT_EQUALS("", errout.str()); + + check("bool f();\n" + "void foo() { bool x = true; if(x&&f()) {}}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always true\n", errout.str()); + } + void pointerAdditionResultNotNull() { check("void f(char *ptr) {\n" " if (ptr + 1 != 0);\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 70222d766..25af86382 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -559,6 +559,56 @@ private: ASSERT_EQUALS(1U, values.size()); ASSERT_EQUALS(-10, values.back().intvalue); + // Logical and + code = "void f(bool b) {\n" + " bool x = false && b;\n" + " bool a = x;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, 0)); + + code = "void f(bool b) {\n" + " bool x = b && false;\n" + " bool a = x;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, 0)); + + code = "void f(bool b) {\n" + " bool x = true && b;\n" + " bool a = x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 3U, 1)); + + code = "void f(bool b) {\n" + " bool x = b && true;\n" + " bool a = x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 3U, 1)); + + // Logical or + code = "void f(bool b) {\n" + " bool x = true || b;\n" + " bool a = x;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, 1)); + + code = "void f(bool b) {\n" + " bool x = b || true;\n" + " bool a = x;\n" + "}"; + ASSERT_EQUALS(true, testValueOfX(code, 3U, 1)); + + code = "void f(bool b) {\n" + " bool x = false || b;\n" + " bool a = x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 3U, 0)); + + code = "void f(bool b) {\n" + " bool x = b || false;\n" + " bool a = x;\n" + "}"; + ASSERT_EQUALS(false, testValueOfX(code, 3U, 0)); + // function call => calculation code = "void f(int x) {\n" " a = x + 8;\n"