From b1d1869f22fe24423ee740cb87179028e130bfce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Mon, 14 Dec 2015 20:03:40 +0100 Subject: [PATCH] minor tweak of comment --- addons/cppcheckdata.py | 2 +- lib/astutils.cpp | 45 ++++++++++ lib/astutils.h | 1 + lib/checkcondition.cpp | 182 +++++++++++++++++++++++++++++++++++++++++ lib/checkcondition.h | 3 + 5 files changed, 232 insertions(+), 1 deletion(-) diff --git a/addons/cppcheckdata.py b/addons/cppcheckdata.py index 0c5c3acf9..3fedc11f4 100644 --- a/addons/cppcheckdata.py +++ b/addons/cppcheckdata.py @@ -352,7 +352,7 @@ class ValueFlow: # http://cppcheck.sourceforge.net/devinfo/doxyoutput/classValueFlow_1_1Value.html class Value: - # #integer value + ## integer value intvalue = None ## token value tokvalue = None diff --git a/lib/astutils.cpp b/lib/astutils.cpp index f7389d749..889c91eb6 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -239,6 +239,51 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token (comp1 == ">" && comp2 == "<")))); } + +bool isIncorrectCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const std::set &constFunctions) +{ + if (!cond1 || !cond2) + return false; + + if (cond1->str() == "!") { + if (cond2->str() == "!=") { + if (cond2->astOperand1() && cond2->astOperand1()->str() == "0") + return isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand2(), constFunctions); + if (cond2->astOperand2() && cond2->astOperand2()->str() == "0") + return isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand1(), constFunctions); + } + return isSameExpression(cpp, cond1->astOperand1(), cond2, constFunctions); + } + + if (cond2->str() == "!") + return isIncorrectCond(isNot, cpp, cond2, cond1, constFunctions); + + if (!cond1->isComparisonOp() || !cond2->isComparisonOp()) + return false; + + const std::string &comp1 = cond1->str(); + + // condition found .. get comparator + std::string comp2; + if (isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand1(), constFunctions) && + isSameExpression(cpp, cond1->astOperand2(), cond2->astOperand2(), constFunctions)) { + comp2 = cond2->str(); + } else if (isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand2(), constFunctions) && + isSameExpression(cpp, cond1->astOperand2(), cond2->astOperand1(), constFunctions)) { + comp2 = cond2->str(); + if (comp2[0] == '>') + comp2[0] = '<'; + else if (comp2[0] == '<') + comp2[0] = '>'; + } + + // is condition incorrect? + return ((comp1 == "<" && comp2 == "==") || + (comp1 == ">" && comp2 == "==") || + (comp1 == "==" && comp2 == "<") || + (comp1 == "==" && comp2 == ">")); +} + bool isConstExpression(const Token *tok, const std::set &constFunctions) { if (!tok) diff --git a/lib/astutils.h b/lib/astutils.h index 32296a5fa..f3a68dcfd 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -59,6 +59,7 @@ bool isSameExpression(bool cpp, const Token *tok1, const Token *tok2, const std: * @param constFunctions constFunctions */ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const std::set &constFunctions); +bool isIncorrectCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const std::set &constFunctions); bool isConstExpression(const Token *tok, const std::set &constFunctions); diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 0d6f1da6e..f7dd40705 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -641,6 +641,187 @@ static std::string conditionString(bool not1, const Token *expr1, const std::str (expr1->isName() ? expr1->str() : std::string("EXPR")); } +void CheckCondition::checkIncorrectCondition() +{ + const bool printStyle = _settings->isEnabled("style"); + const bool printWarning = _settings->isEnabled("warning"); + if (!printWarning && !printStyle) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t ii = 0; ii < functions; ++ii) { + const Scope * scope = symbolDatabase->functionScopes[ii]; + + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + if (!Token::Match(tok, "%oror%|&&") || !tok->astOperand1() || !tok->astOperand2()) + continue; + + // Opposite comparisons around || or && => always true or always false + if ((tok->astOperand1()->isName() || tok->astOperand2()->isName()) && + isIncorrectCond(true, _tokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) { + + const bool alwaysTrue(tok->str() == "||"); + incorrectConditionError(tok, tok->expressionString(), alwaysTrue); + continue; + } + + + // 'A && (!A || B)' is equivalent with 'A && B' + // 'A || (!A && B)' is equivalent with 'A || B' + if (printStyle && + ((tok->str() == "||" && tok->astOperand2()->str() == "&&") || + (tok->str() == "&&" && tok->astOperand2()->str() == "||"))) { + const Token* tok2 = tok->astOperand2()->astOperand1(); + if (isIncorrectCond(true, _tokenizer->isCPP(), tok->astOperand1(), tok2, _settings->library.functionpure)) { + std::string expr1(tok->astOperand1()->expressionString()); + std::string expr2(tok->astOperand2()->astOperand1()->expressionString()); + std::string expr3(tok->astOperand2()->astOperand2()->expressionString()); + + if (expr1.length() + expr2.length() + expr3.length() > 50U) { + if (expr1[0] == '!' && expr2[0] != '!') { + expr1 = "!A"; + expr2 = "A"; + } else { + expr1 = "A"; + expr2 = "!A"; + } + + expr3 = "B"; + } + + const std::string cond1 = expr1 + " " + tok->str() + " (" + expr2 + " " + tok->astOperand2()->str() + " " + expr3 + ")"; + const std::string cond2 = expr1 + " " + tok->str() + " " + expr3; + + redundantConditionError(tok, tok2->expressionString() + ". '" + cond1 + "' is equivalent to '" + cond2 + "'"); + continue; + } + } + + // Comparison #1 (LHS) + const Token *comp1 = tok->astOperand1(); + if (comp1 && comp1->str() == tok->str()) + comp1 = comp1->astOperand2(); + + // Comparison #2 (RHS) + const Token *comp2 = tok->astOperand2(); + + // Parse LHS + bool not1; + std::string op1, value1; + const Token *expr1; + if (!parseComparison(comp1, ¬1, &op1, &value1, &expr1)) + continue; + + // Parse RHS + bool not2; + std::string op2, value2; + const Token *expr2; + if (!parseComparison(comp2, ¬2, &op2, &value2, &expr2)) + continue; + + if (isSameExpression(_tokenizer->isCPP(), comp1, comp2, _settings->library.functionpure)) + continue; // same expressions => only report that there are same expressions + if (!isSameExpression(_tokenizer->isCPP(), expr1, expr2, _settings->library.functionpure)) { + if ((tok->astOperand1()->isConstOp() && tok->astOperand2()->isConstOp()) && + isIncorrectCond(true, _tokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) { + + const bool alwaysTrue(tok->str() == "||"); + incorrectConditionError(tok, tok->expressionString(), alwaysTrue); + } + continue; + } + + const bool isfloat = astIsFloat(expr1, true) || MathLib::isFloat(value1) || astIsFloat(expr2, true) || MathLib::isFloat(value2); + + // don't check floating point equality comparisons. that is bad + // and deserves different warnings. + if (isfloat && (op1 == "==" || op1 == "!=" || op2 == "==" || op2 == "!=")) + continue; + + const double d1 = (isfloat) ? MathLib::toDoubleNumber(value1) : 0; + const double d2 = (isfloat) ? MathLib::toDoubleNumber(value2) : 0; + const MathLib::bigint i1 = (isfloat) ? 0 : MathLib::toLongNumber(value1); + const MathLib::bigint i2 = (isfloat) ? 0 : MathLib::toLongNumber(value2); + const bool useUnsignedInt = (std::numeric_limits::max()==i1)||(std::numeric_limits::max()==i2); + const MathLib::biguint u1 = (useUnsignedInt) ? MathLib::toLongNumber(value1) : 0; + const MathLib::biguint u2 = (useUnsignedInt) ? MathLib::toLongNumber(value2) : 0; + // evaluate if expression is always true/false + bool alwaysTrue = true, alwaysFalse = true; + bool firstTrue = true, secondTrue = true; + for (int test = 1; test <= 5; ++test) { + // test: + // 1 => testvalue is less than both value1 and value2 + // 2 => testvalue is value1 + // 3 => testvalue is between value1 and value2 + // 4 => testvalue value2 + // 5 => testvalue is larger than both value1 and value2 + bool result1, result2; + if (isfloat) { + const double testvalue = getvalue(test, d1, d2); + result1 = checkFloatRelation(op1, testvalue, d1); + result2 = checkFloatRelation(op2, testvalue, d2); + } else if (useUnsignedInt) { + const MathLib::biguint testvalue = getvalue(test, u1, u2); + result1 = checkIntRelation(op1, testvalue, u1); + result2 = checkIntRelation(op2, testvalue, u2); + } else { + const MathLib::bigint testvalue = getvalue(test, i1, i2); + result1 = checkIntRelation(op1, testvalue, i1); + result2 = checkIntRelation(op2, testvalue, i2); + } + if (not1) + result1 = !result1; + if (not2) + result2 = !result2; + if (tok->str() == "&&") { + alwaysTrue &= (result1 && result2); + alwaysFalse &= !(result1 && result2); + } else { + alwaysTrue &= (result1 || result2); + alwaysFalse &= !(result1 || result2); + } + firstTrue &= !(!result1 && result2); + secondTrue &= !(result1 && !result2); + } + + const std::string cond1str = conditionString(not1, expr1, op1, value1); + const std::string cond2str = conditionString(not2, expr2, op2, value2); + if (printWarning && (alwaysTrue || alwaysFalse)) { + const std::string text = cond1str + " " + tok->str() + " " + cond2str; + incorrectConditionError(tok, text, alwaysTrue); + } else if (printStyle && secondTrue) { + const std::string text = "If '" + cond1str + "', the comparison '" + cond2str + + "' is always " + (secondTrue ? "true" : "false") + "."; + redundantConditionError(tok, text); + } 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 " + (firstTrue ? "true" : "false") + "."; + redundantConditionError(tok, text); + } + } + } +} + +void CheckCondition::incorrectConditionError(const Token *tok, const std::string &condition, bool always) +{ + if (always) + reportError(tok, Severity::warning, "IncorrectCondition", + "Condition always evaluates to true: " + condition + ".\n" + "Condition 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?"); + else + reportError(tok, Severity::warning, "IncorrectCondition", + "Condition always evaluates to false: " + condition + ".\n" + "Condition 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?"); +} + + + void CheckCondition::checkIncorrectLogicOperator() { const bool printStyle = _settings->isEnabled("style"); @@ -813,6 +994,7 @@ void CheckCondition::incorrectLogicOperatorError(const Token *tok, const std::st "Are these conditions necessary? Did you intend to use || instead? Are the numbers correct? Are you comparing the correct variables?"); } + void CheckCondition::redundantConditionError(const Token *tok, const std::string &text) { reportError(tok, Severity::style, "redundantCondition", "Redundant condition: " + text); diff --git a/lib/checkcondition.h b/lib/checkcondition.h index acc4e313b..8d48dcb77 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -50,6 +50,7 @@ public: checkCondition.clarifyCondition(); // not simplified because ifAssign checkCondition.oppositeInnerCondition(); checkCondition.checkIncorrectLogicOperator(); + checkCondition.checkIncorrectCondition(); checkCondition.checkInvalidTestForOverflow(); } @@ -88,6 +89,7 @@ public: /** @brief %Check for testing for mutual exclusion over ||*/ void checkIncorrectLogicOperator(); + void checkIncorrectCondition(); /** @brief %Check for suspicious usage of modulo (e.g. "if(var % 4 == 4)") */ void checkModuloAlwaysTrueFalse(); @@ -118,6 +120,7 @@ private: void oppositeInnerConditionError(const Token *tok1, const Token* tok2); void incorrectLogicOperatorError(const Token *tok, const std::string &condition, bool always); + void incorrectConditionError(const Token *tok, const std::string &condition, bool always); void redundantConditionError(const Token *tok, const std::string &text); void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal);