From 1fe1ec09a8cdb1512f119a823aa002954a5c0c06 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 1 Oct 2018 07:40:03 -0500 Subject: [PATCH] Reenable follow var for logical conjunction (#1400) --- lib/astutils.cpp | 8 ++++---- lib/checkcondition.cpp | 19 +++++++++++-------- lib/checkcondition.h | 4 ++-- test/testcondition.cpp | 4 ++-- 4 files changed, 19 insertions(+), 16 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 6a09a8d70..59985e954 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -481,11 +481,11 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token // condition found .. get comparator std::string comp2; - if (isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand1(), library, pure, followVar) && - isSameExpression(cpp, true, cond1->astOperand2(), cond2->astOperand2(), library, pure, followVar)) { + if (isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand1(), library, pure, followVar, errors) && + isSameExpression(cpp, true, cond1->astOperand2(), cond2->astOperand2(), library, pure, followVar, errors)) { comp2 = cond2->str(); - } else if (isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand2(), library, pure, followVar) && - isSameExpression(cpp, true, cond1->astOperand2(), cond2->astOperand1(), library, pure, followVar)) { + } else if (isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand2(), library, pure, followVar, errors) && + isSameExpression(cpp, true, cond1->astOperand2(), cond2->astOperand1(), library, pure, followVar, errors)) { comp2 = cond2->str(); if (comp2[0] == '>') comp2[0] = '<'; diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 6f8339135..9a2e67710 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -973,20 +973,22 @@ void CheckCondition::checkIncorrectLogicOperator() const bool isfloat = astIsFloat(expr1, true) || MathLib::isFloat(value1) || astIsFloat(expr2, true) || MathLib::isFloat(value2); + ErrorPath errorPath; + // Opposite comparisons around || or && => always true or always false - if (!isfloat && isOppositeCond(tok->str() == "||", mTokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), mSettings->library, true, false)) { + if (!isfloat && isOppositeCond(tok->str() == "||", mTokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), mSettings->library, true, true, &errorPath)) { const bool alwaysTrue(tok->str() == "||"); - incorrectLogicOperatorError(tok, conditionString(tok), alwaysTrue, inconclusive); + incorrectLogicOperatorError(tok, conditionString(tok), alwaysTrue, inconclusive, errorPath); continue; } if (!parseable) continue; - if (isSameExpression(mTokenizer->isCPP(), true, comp1, comp2, mSettings->library, true, false)) + if (isSameExpression(mTokenizer->isCPP(), true, comp1, comp2, mSettings->library, true, true)) continue; // same expressions => only report that there are same expressions - if (!isSameExpression(mTokenizer->isCPP(), true, expr1, expr2, mSettings->library, true, false)) + if (!isSameExpression(mTokenizer->isCPP(), true, expr1, expr2, mSettings->library, true, true)) continue; @@ -1046,7 +1048,7 @@ void CheckCondition::checkIncorrectLogicOperator() const std::string cond2str = conditionString(not2, expr2, op2, value2); if (printWarning && (alwaysTrue || alwaysFalse)) { const std::string text = cond1str + " " + tok->str() + " " + cond2str; - incorrectLogicOperatorError(tok, text, alwaysTrue, inconclusive); + incorrectLogicOperatorError(tok, text, alwaysTrue, inconclusive, errorPath); } else if (printStyle && secondTrue) { const std::string text = "If '" + cond1str + "', the comparison '" + cond2str + "' is always true."; @@ -1063,15 +1065,16 @@ void CheckCondition::checkIncorrectLogicOperator() } } -void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always, bool inconclusive) +void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always, bool inconclusive, ErrorPath errors) { + errors.emplace_back(tok, ""); if (always) - reportError(tok, Severity::warning, "incorrectLogicOperator", + reportError(errors, Severity::warning, "incorrectLogicOperator", "Logical disjunction always evaluates to true: " + condition + ".\n" "Logical disjunction always evaluates to true: " + condition + ". " "Are these conditions necessary? Did you intend to use && instead? Are the numbers correct? Are you comparing the correct variables?", CWE571, inconclusive); else - reportError(tok, Severity::warning, "incorrectLogicOperator", + reportError(errors, Severity::warning, "incorrectLogicOperator", "Logical conjunction always evaluates to false: " + condition + ".\n" "Logical conjunction always evaluates to false: " + condition + ". " "Are these conditions necessary? Did you intend to use || instead? Are the numbers correct? Are you comparing the correct variables?", CWE570, inconclusive); diff --git a/lib/checkcondition.h b/lib/checkcondition.h index ac6e08a5f..5c436895e 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -137,7 +137,7 @@ private: void identicalConditionAfterEarlyExitError(const Token *cond1, const Token *cond2, ErrorPath errorPath); - void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always, bool inconclusive); + void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always, bool inconclusive, ErrorPath errors); void redundantConditionError(const Token *tok, const std::string &text, bool inconclusive); void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal); @@ -162,7 +162,7 @@ private: c.oppositeInnerConditionError(nullptr, nullptr, errorPath); c.identicalInnerConditionError(nullptr, nullptr, errorPath); c.identicalConditionAfterEarlyExitError(nullptr, nullptr, errorPath); - c.incorrectLogicOperatorError(nullptr, "foo > 3 && foo < 4", true, false); + c.incorrectLogicOperatorError(nullptr, "foo > 3 && foo < 4", true, false, errorPath); c.redundantConditionError(nullptr, "If x > 11 the condition x > 10 is always true.", false); c.moduloAlwaysTrueFalseError(nullptr, "1"); c.clarifyConditionError(nullptr, true, false); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 8d89222a7..7e2de11ea 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -1246,7 +1246,7 @@ private: " if (a > x && a < y)\n" " return;\n" "}\n"); - // ASSERT_EQUALS("[test.cpp:8]: (warning) Logical conjunction always evaluates to false: a > x && a < y.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:6] -> [test.cpp:8]: (warning) Logical conjunction always evaluates to false: a > x && a < y.\n", errout.str()); check("struct A {\n" " void f();\n" @@ -1276,7 +1276,7 @@ private: " if (a > x && a < y)\n" " return;\n" "}\n"); - // ASSERT_EQUALS("[test.cpp:5]: (warning) Logical conjunction always evaluates to false: a > x && a < y.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3] -> [test.cpp:5]: (warning) Logical conjunction always evaluates to false: a > x && a < y.\n", errout.str()); } void secondAlwaysTrueFalseWhenFirstTrueError() {