From 24438c326e43284d12c94fb56d1ecff294305584 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Fri, 25 Dec 2015 18:31:21 +0100 Subject: [PATCH] Fixed #7233 (Fasle negative 'unknownEvaluationOrder in case of macro) --- lib/astutils.cpp | 30 +++++++++++++++--------------- lib/astutils.h | 2 +- lib/checkcondition.cpp | 12 ++++++------ lib/checkother.cpp | 17 +++++++++-------- test/testother.cpp | 7 +++++++ 5 files changed, 38 insertions(+), 30 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index f7389d749..647c913ce 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -98,7 +98,7 @@ const Token * astIsVariableComparison(const Token *tok, const std::string &comp, return ret; } -bool isSameExpression(bool cpp, const Token *tok1, const Token *tok2, const std::set &constFunctions) +bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const std::set &constFunctions) { if (tok1 == nullptr && tok2 == nullptr) return true; @@ -113,14 +113,14 @@ bool isSameExpression(bool cpp, const Token *tok1, const Token *tok2, const std: if (tok1->varId() != tok2->varId() || tok1->str() != tok2->str()) { if ((Token::Match(tok1,"<|>") && Token::Match(tok2,"<|>")) || (Token::Match(tok1,"<=|>=") && Token::Match(tok2,"<=|>="))) { - return isSameExpression(cpp, tok1->astOperand1(), tok2->astOperand2(), constFunctions) && - isSameExpression(cpp, tok1->astOperand2(), tok2->astOperand1(), constFunctions); + return isSameExpression(cpp, macro, tok1->astOperand1(), tok2->astOperand2(), constFunctions) && + isSameExpression(cpp, macro, tok1->astOperand2(), tok2->astOperand1(), constFunctions); } return false; } if (tok1->str() == "." && tok1->originalName() != tok2->originalName()) return false; - if (tok1->isExpandedMacro() || tok2->isExpandedMacro()) + if (macro && (tok1->isExpandedMacro() || tok2->isExpandedMacro())) return false; if (tok1->isName() && tok1->next()->str() == "(" && tok1->str() != "sizeof") { if (!tok1->function() && !Token::Match(tok1->previous(), ".|::") && constFunctions.find(tok1->str()) == constFunctions.end() && !tok1->isAttributeConst() && !tok1->isAttributePure()) @@ -168,18 +168,18 @@ bool isSameExpression(bool cpp, const Token *tok1, const Token *tok2, const std: return false; } bool noncommuative_equals = - isSameExpression(cpp, tok1->astOperand1(), tok2->astOperand1(), constFunctions); + isSameExpression(cpp, macro, tok1->astOperand1(), tok2->astOperand1(), constFunctions); noncommuative_equals = noncommuative_equals && - isSameExpression(cpp, tok1->astOperand2(), tok2->astOperand2(), constFunctions); + isSameExpression(cpp, macro, tok1->astOperand2(), tok2->astOperand2(), constFunctions); if (noncommuative_equals) return true; const bool commutative = tok1->astOperand1() && tok1->astOperand2() && Token::Match(tok1, "%or%|%oror%|+|*|&|&&|^|==|!="); bool commuative_equals = commutative && - isSameExpression(cpp, tok1->astOperand2(), tok2->astOperand1(), constFunctions); + isSameExpression(cpp, macro, tok1->astOperand2(), tok2->astOperand1(), constFunctions); commuative_equals = commuative_equals && - isSameExpression(cpp, tok1->astOperand1(), tok2->astOperand2(), constFunctions); + isSameExpression(cpp, macro, tok1->astOperand1(), tok2->astOperand2(), constFunctions); // in c++, "a"+b might be different to b+"a" if (cpp && commuative_equals && tok1->str() == "+" && @@ -199,11 +199,11 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token if (cond1->str() == "!") { if (cond2->str() == "!=") { if (cond2->astOperand1() && cond2->astOperand1()->str() == "0") - return isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand2(), constFunctions); + return isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand2(), constFunctions); if (cond2->astOperand2() && cond2->astOperand2()->str() == "0") - return isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand1(), constFunctions); + return isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand1(), constFunctions); } - return isSameExpression(cpp, cond1->astOperand1(), cond2, constFunctions); + return isSameExpression(cpp, true, cond1->astOperand1(), cond2, constFunctions); } if (cond2->str() == "!") @@ -216,11 +216,11 @@ bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token // condition found .. get comparator std::string comp2; - if (isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand1(), constFunctions) && - isSameExpression(cpp, cond1->astOperand2(), cond2->astOperand2(), constFunctions)) { + if (isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand1(), constFunctions) && + isSameExpression(cpp, true, cond1->astOperand2(), cond2->astOperand2(), constFunctions)) { comp2 = cond2->str(); - } else if (isSameExpression(cpp, cond1->astOperand1(), cond2->astOperand2(), constFunctions) && - isSameExpression(cpp, cond1->astOperand2(), cond2->astOperand1(), constFunctions)) { + } else if (isSameExpression(cpp, true, cond1->astOperand1(), cond2->astOperand2(), constFunctions) && + isSameExpression(cpp, true, cond1->astOperand2(), cond2->astOperand1(), constFunctions)) { comp2 = cond2->str(); if (comp2[0] == '>') comp2[0] = '<'; diff --git a/lib/astutils.h b/lib/astutils.h index 32296a5fa..d640faf96 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -48,7 +48,7 @@ std::string astCanonicalType(const Token *expr); /** Is given syntax tree a variable comparison against value */ const Token * astIsVariableComparison(const Token *tok, const std::string &comp, const std::string &rhs, const Token **vartok=nullptr); -bool isSameExpression(bool cpp, const Token *tok1, const Token *tok2, const std::set &constFunctions); +bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const std::set &constFunctions); /** * Are two conditions opposite diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 0d6f1da6e..b810f3a34 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -326,7 +326,7 @@ bool CheckCondition::isOverlappingCond(const Token * const cond1, const Token * return false; // same expressions - if (isSameExpression(_tokenizer->isCPP(), cond1,cond2,constFunctions)) + if (isSameExpression(_tokenizer->isCPP(), true, cond1, cond2, constFunctions)) return true; // bitwise overlap for example 'x&7' and 'x==1' @@ -349,7 +349,7 @@ bool CheckCondition::isOverlappingCond(const Token * const cond1, const Token * if (!num2->isNumber() || MathLib::isNegative(num2->str())) return false; - if (!isSameExpression(_tokenizer->isCPP(), expr1,expr2,constFunctions)) + if (!isSameExpression(_tokenizer->isCPP(), true, expr1, expr2, constFunctions)) return false; const MathLib::bigint value1 = MathLib::toLongNumber(num1->str()); @@ -720,9 +720,9 @@ void CheckCondition::checkIncorrectLogicOperator() if (!parseComparison(comp2, ¬2, &op2, &value2, &expr2)) continue; - if (isSameExpression(_tokenizer->isCPP(), comp1, comp2, _settings->library.functionpure)) + if (isSameExpression(_tokenizer->isCPP(), true, 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 (!isSameExpression(_tokenizer->isCPP(), true, expr1, expr2, _settings->library.functionpure)) continue; const bool isfloat = astIsFloat(expr1, true) || MathLib::isFloat(value1) || astIsFloat(expr2, true) || MathLib::isFloat(value2); @@ -1049,9 +1049,9 @@ void CheckCondition::checkInvalidTestForOverflow() continue; const Token *termToken; - if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand1(), _settings->library.functionpure)) + if (isSameExpression(_tokenizer->isCPP(), true, exprToken, calcToken->astOperand1(), _settings->library.functionpure)) termToken = calcToken->astOperand2(); - else if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand2(), _settings->library.functionpure)) + else if (isSameExpression(_tokenizer->isCPP(), true, exprToken, calcToken->astOperand2(), _settings->library.functionpure)) termToken = calcToken->astOperand1(); else continue; diff --git a/lib/checkother.cpp b/lib/checkother.cpp index c7376031b..d369a8a6c 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1890,7 +1890,7 @@ void CheckOther::checkDuplicateExpression() if (tok->isOp() && tok->astOperand1() && !Token::Match(tok, "+|*|<<|>>|+=|*=|<<=|>>=")) { if (Token::Match(tok, "==|!=|-") && astIsFloat(tok->astOperand1(), true)) continue; - if (isSameExpression(_tokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) { + if (isSameExpression(_tokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), _settings->library.functionpure)) { if (isWithoutSideEffects(_tokenizer->isCPP(), tok->astOperand1())) { const bool assignment = tok->str() == "="; if (assignment && warningEnabled) @@ -1910,16 +1910,16 @@ void CheckOther::checkDuplicateExpression() } } } else if (!Token::Match(tok, "[-/%]")) { // These operators are not associative - if (styleEnabled && tok->astOperand2() && tok->str() == tok->astOperand1()->str() && isSameExpression(_tokenizer->isCPP(), tok->astOperand2(), tok->astOperand1()->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer->isCPP(), tok->astOperand2())) + if (styleEnabled && tok->astOperand2() && tok->str() == tok->astOperand1()->str() && isSameExpression(_tokenizer->isCPP(), true, tok->astOperand2(), tok->astOperand1()->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer->isCPP(), tok->astOperand2())) duplicateExpressionError(tok->astOperand2(), tok->astOperand2(), tok->str()); else if (tok->astOperand2()) { const Token *ast1 = tok->astOperand1(); while (ast1 && tok->str() == ast1->str()) { - if (isSameExpression(_tokenizer->isCPP(), ast1->astOperand1(), tok->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer->isCPP(), ast1->astOperand1())) + if (isSameExpression(_tokenizer->isCPP(), true, ast1->astOperand1(), tok->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer->isCPP(), ast1->astOperand1())) // TODO: warn if variables are unchanged. See #5683 // Probably the message should be changed to 'duplicate expressions X in condition or something like that'. ;//duplicateExpressionError(ast1->astOperand1(), tok->astOperand2(), tok->str()); - else if (styleEnabled && isSameExpression(_tokenizer->isCPP(), ast1->astOperand2(), tok->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer->isCPP(), ast1->astOperand2())) + else if (styleEnabled && isSameExpression(_tokenizer->isCPP(), true, ast1->astOperand2(), tok->astOperand2(), _settings->library.functionpure) && isWithoutSideEffects(_tokenizer->isCPP(), ast1->astOperand2())) duplicateExpressionError(ast1->astOperand2(), tok->astOperand2(), tok->str()); if (!isConstExpression(ast1->astOperand2(), _settings->library.functionpure)) break; @@ -1928,7 +1928,7 @@ void CheckOther::checkDuplicateExpression() } } } else if (styleEnabled && tok->astOperand1() && tok->astOperand2() && tok->str() == ":" && tok->astParent() && tok->astParent()->str() == "?") { - if (isSameExpression(_tokenizer->isCPP(), tok->astOperand1(), tok->astOperand2(), temp)) + if (isSameExpression(_tokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), temp)) duplicateExpressionTernaryError(tok); } } @@ -2515,8 +2515,9 @@ void CheckOther::checkEvaluationOrder() if (tok2 == tok && tok->str() == "=" && parent->str() == "=" && - isSameExpression(_tokenizer->isCPP(), tok->astOperand1(), parent->astOperand1(), _settings->library.functionpure)) { - if (_settings->isEnabled("warning")) + isSameExpression(_tokenizer->isCPP(), false, tok->astOperand1(), parent->astOperand1(), _settings->library.functionpure)) { + if (_settings->isEnabled("warning") && + isSameExpression(_tokenizer->isCPP(), true, tok->astOperand1(), parent->astOperand1(), _settings->library.functionpure)) selfAssignmentError(parent, tok->astOperand1()->expressionString()); break; } @@ -2532,7 +2533,7 @@ void CheckOther::checkEvaluationOrder() continue; tokens.push(tok3->astOperand1()); tokens.push(tok3->astOperand2()); - if (isSameExpression(_tokenizer->isCPP(), tok->astOperand1(), tok3, _settings->library.functionpure)) { + if (isSameExpression(_tokenizer->isCPP(), false, tok->astOperand1(), tok3, _settings->library.functionpure)) { foundError = true; } } diff --git a/test/testother.cpp b/test/testother.cpp index 963493bd9..0f3e45c5f 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -6118,6 +6118,13 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) Redundant assignment of 'x' to itself.\n", errout.str()); + // macro, dont bailout (#7233) + check((std::string("void f(int x) {\n" + " return x + ") + Preprocessor::macroChar + "x++;\n" + "}").c_str()); + ASSERT_EQUALS("[test.cpp:2]: (error) Expression 'x+x++' depends on order of evaluation of side effects\n", errout.str()); + + // sequence points { // FP