From 71e2f6c7c5210f965f441122f8f235421228316d Mon Sep 17 00:00:00 2001 From: Oleksandr Labetskyi Date: Fri, 10 May 2024 14:25:17 +0300 Subject: [PATCH 1/6] First draft --- lib/checkother.cpp | 91 +++++++++++++++++++++++++++++++++++++++++----- lib/checkother.h | 5 ++- test/testother.cpp | 11 ++++++ 3 files changed, 96 insertions(+), 11 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index ab819ce857b..c380cf48c0e 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3265,15 +3265,9 @@ void CheckOther::unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef Certainty::normal); } - -void CheckOther::checkEvaluationOrder() +void CheckOther::checkEvaluationOrderPre11() { - // This checker is not written according to C++11 sequencing rules - if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11) - return; - - logChecker("CheckOther::checkEvaluationOrder"); // C/C++03 - + logChecker("CheckOther::checkEvaluationOrder"); const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Scope * functionScope : symbolDatabase->functionScopes) { for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { @@ -3341,9 +3335,86 @@ void CheckOther::checkEvaluationOrder() } } -void CheckOther::unknownEvaluationOrder(const Token* tok) +void CheckOther::checkEvaluationOrderPost11() +{ + return; +} + +void CheckOther::checkEvaluationOrderPost17() +{ + logChecker("CheckOther::checkEvaluationOrder"); + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope * functionScope : symbolDatabase->functionScopes) { + for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { + if (tok->tokType() != Token::eIncDecOp || !tok->astOperand1()) + continue; + for (const Token *tok2 = tok;; tok2 = tok2->astParent()) { + const Token * const parent = tok2->astParent(); + if (!parent || !parent->isBinaryOp()) + break; + if (parent->str() == ",") { + const Token *par = parent; + while (Token::simpleMatch(par,",")) + par = par->astParent(); + // not function or in a while clause => break + if (!(par && par->str() == "(" && par->astOperand2() && par->strAt(-1) != "while")) + break; + // control flow (if|while|etc) => break + if (Token::simpleMatch(par->link(),") {")) + break; + // sequence point in function argument: dostuff((1,2),3) => break + par = par->next(); + while (par && (par->previous() != parent)) + par = par->nextArgument(); + if (!par) + break; + } + if (parent->str() == "(" && parent->astOperand2()) + break; + + // Is expression used? + bool foundUndefined{false}, foundUnspecified{false}; + visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), + [&](const Token *tok3) { + if (tok3->str() == "&" && !tok3->astOperand2()) + return ChildrenToVisit::none; // don't handle address-of for now + if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof")) + return ChildrenToVisit::none; // don't care about sizeof usage + if (isSameExpression(false, tok->astOperand1(), tok3, *mSettings, true, false) && parent->isArithmeticalOp()) + foundUndefined = true; + if (tok3->tokType() == Token::eIncDecOp && isSameExpression(false, tok->astOperand1(), tok3->astOperand1(), *mSettings, true, false)) { + if (parent->isArithmeticalOp()) + foundUndefined = true; + else + foundUnspecified = true; + } + return (foundUndefined || foundUnspecified) ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; + }); + + if (foundUndefined || foundUnspecified) { + unknownEvaluationOrder(parent, !foundUndefined); + break; + } + } + } + } +} + +void CheckOther::checkEvaluationOrder() +{ + if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11) { + if (mSettings->standards.cpp >= Standards::CPP17) + checkEvaluationOrderPost17(); + if (mSettings->standards.cpp >= Standards::CPP11) + checkEvaluationOrderPost11(); + } + else + checkEvaluationOrderPre11(); +} + +void CheckOther::unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior) { - reportError(tok, Severity::error, "unknownEvaluationOrder", + reportError(tok, isUnspecifiedBehavior ? Severity::portability : Severity::error, "unknownEvaluationOrder", "Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects", CWE768, Certainty::normal); } diff --git a/lib/checkother.h b/lib/checkother.h index 79b62e96988..49651525ef0 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -212,6 +212,9 @@ class CPPCHECKLIB CheckOther : public Check { /** @brief %Check for expression that depends on order of evaluation of side effects */ void checkEvaluationOrder(); + void checkEvaluationOrderPost17(); + void checkEvaluationOrderPost11(); + void checkEvaluationOrderPre11(); /** @brief %Check for access of moved or forwarded variable */ void checkAccessOfMovedVariable(); @@ -280,7 +283,7 @@ class CPPCHECKLIB CheckOther : public Check { void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive, bool addressOfDeref); void raceAfterInterlockedDecrementError(const Token* tok); void unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef); - void unknownEvaluationOrder(const Token* tok); + void unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior = false); void accessMovedError(const Token *tok, const std::string &varname, const ValueFlow::Value *value, bool inconclusive); void funcArgNamesDifferent(const std::string & functionName, nonneg int index, const Token* declaration, const Token* definition); void funcArgOrderDifferent(const std::string & functionName, const Token * declaration, const Token * definition, const std::vector & declarations, const std::vector & definitions); diff --git a/test/testother.cpp b/test/testother.cpp index bc9ea2861e9..6c15138ba4f 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -10842,6 +10842,17 @@ class TestOther : public TestFixture { " a[x+y] = a[y+x]++;;\n" "}\n", false); ASSERT_EQUALS("[test.c:3]: (error) Expression 'a[x+y]=a[y+x]++' depends on order of evaluation of side effects\n", errout_str()); + + check("void f(int i) {\n" + " int n = ++i + i;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) Expression '++i+i' depends on order of evaluation of side effects\n", errout_str()); + + check("long int f1(const char *exp) {\n" + " return dostuff(++exp, ++exp, 10);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (portability) Expression '++exp,++exp' depends on order of evaluation of side effects\n" + "[test.cpp:2]: (portability) Expression '++exp,++exp' depends on order of evaluation of side effects\n", errout_str()); } void testEvaluationOrderSelfAssignment() { From d50cd7b5cc3ec76ebfd9577fbd6b5920fc1123c5 Mon Sep 17 00:00:00 2001 From: Oleksandr Labetskyi Date: Mon, 13 May 2024 12:04:41 +0300 Subject: [PATCH 2/6] Fixes --- lib/checkother.cpp | 30 +++++++++++++++++++++++------- test/testother.cpp | 9 +++++++-- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index c380cf48c0e..fffa8544536 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3265,6 +3265,17 @@ void CheckOther::unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef Certainty::normal); } +static bool isFunctionCall(const Token* tok) +{ + if (Token::Match(tok, "%name% (")) + return true; + if (Token::Match(tok, "%name% <") && Token::simpleMatch(tok->next()->link(), "> (")) + return true; + if (Token::Match(tok, "%name% ::")) + return isFunctionCall(tok->tokAt(2)); + return false; +} + void CheckOther::checkEvaluationOrderPre11() { logChecker("CheckOther::checkEvaluationOrder"); @@ -3337,7 +3348,7 @@ void CheckOther::checkEvaluationOrderPre11() void CheckOther::checkEvaluationOrderPost11() { - return; + // TODO } void CheckOther::checkEvaluationOrderPost17() @@ -3350,7 +3361,9 @@ void CheckOther::checkEvaluationOrderPost17() continue; for (const Token *tok2 = tok;; tok2 = tok2->astParent()) { const Token * const parent = tok2->astParent(); - if (!parent || !parent->isBinaryOp()) + if (!parent) + break; + if (Token::Match(parent, "%oror%|&&|?|:|;")) break; if (parent->str() == ",") { const Token *par = parent; @@ -3380,10 +3393,10 @@ void CheckOther::checkEvaluationOrderPost17() return ChildrenToVisit::none; // don't handle address-of for now if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof")) return ChildrenToVisit::none; // don't care about sizeof usage - if (isSameExpression(false, tok->astOperand1(), tok3, *mSettings, true, false) && parent->isArithmeticalOp()) + if (isSameExpression(false, tok->astOperand1(), tok3, *mSettings, true, false) && parent->isArithmeticalOp() && parent->isBinaryOp()) foundUndefined = true; if (tok3->tokType() == Token::eIncDecOp && isSameExpression(false, tok->astOperand1(), tok3->astOperand1(), *mSettings, true, false)) { - if (parent->isArithmeticalOp()) + if (parent->isArithmeticalOp() && parent->isBinaryOp()) foundUndefined = true; else foundUnspecified = true; @@ -3405,7 +3418,7 @@ void CheckOther::checkEvaluationOrder() if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11) { if (mSettings->standards.cpp >= Standards::CPP17) checkEvaluationOrderPost17(); - if (mSettings->standards.cpp >= Standards::CPP11) + else checkEvaluationOrderPost11(); } else @@ -3414,8 +3427,11 @@ void CheckOther::checkEvaluationOrder() void CheckOther::unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior) { - reportError(tok, isUnspecifiedBehavior ? Severity::portability : Severity::error, "unknownEvaluationOrder", - "Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects", CWE768, Certainty::normal); + isUnspecifiedBehavior ? + reportError(tok, Severity::portability, "unknownEvaluationOrder", + "Expression '" + (tok ? tok->expressionString() : std::string("x++, x++")) + "' depends on order of evaluation of side effects. Behavior is Unspecified according to c++17", CWE768, Certainty::normal) + : reportError(tok, Severity::error, "unknownEvaluationOrder", + "Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects", CWE768, Certainty::normal); } void CheckOther::checkAccessOfMovedVariable() diff --git a/test/testother.cpp b/test/testother.cpp index 6c15138ba4f..52df1238ce5 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -10851,8 +10851,13 @@ class TestOther : public TestFixture { check("long int f1(const char *exp) {\n" " return dostuff(++exp, ++exp, 10);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (portability) Expression '++exp,++exp' depends on order of evaluation of side effects\n" - "[test.cpp:2]: (portability) Expression '++exp,++exp' depends on order of evaluation of side effects\n", errout_str()); + ASSERT_EQUALS("[test.cpp:2]: (portability) Expression '++exp,++exp' depends on order of evaluation of side effects. Behavior is Unspecified according to c++17\n" + "[test.cpp:2]: (portability) Expression '++exp,++exp' depends on order of evaluation of side effects. Behavior is Unspecified according to c++17\n", errout_str()); + + check("void f(int i) {\n" + " int n = (~(-(++i)) + i);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (error) Expression '~(-(++i))+i' depends on order of evaluation of side effects\n", errout_str()); } void testEvaluationOrderSelfAssignment() { From 056140d878d003227aa03e98e4242d03208eb3b6 Mon Sep 17 00:00:00 2001 From: Oleksandr Labetskyi Date: Mon, 13 May 2024 12:23:24 +0300 Subject: [PATCH 3/6] Fixes 2 --- lib/checkother.cpp | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index fffa8544536..88ee5b0ad11 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3265,17 +3265,6 @@ void CheckOther::unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef Certainty::normal); } -static bool isFunctionCall(const Token* tok) -{ - if (Token::Match(tok, "%name% (")) - return true; - if (Token::Match(tok, "%name% <") && Token::simpleMatch(tok->next()->link(), "> (")) - return true; - if (Token::Match(tok, "%name% ::")) - return isFunctionCall(tok->tokAt(2)); - return false; -} - void CheckOther::checkEvaluationOrderPre11() { logChecker("CheckOther::checkEvaluationOrder"); From 013903d8c17b16bdc42fde6db635c74d5d5d28d6 Mon Sep 17 00:00:00 2001 From: Oleksandr Labetskyi Date: Mon, 13 May 2024 13:28:14 +0300 Subject: [PATCH 4/6] Another fix --- lib/checkother.cpp | 158 ++++++++++++++++----------------------------- lib/checkother.h | 3 - 2 files changed, 56 insertions(+), 105 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 88ee5b0ad11..813eba71db3 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3265,90 +3265,65 @@ void CheckOther::unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef Certainty::normal); } -void CheckOther::checkEvaluationOrderPre11() +static bool checkEvaluationOrderPre11(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & selfAssignmentError) { - logChecker("CheckOther::checkEvaluationOrder"); - const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); - for (const Scope * functionScope : symbolDatabase->functionScopes) { - for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { - if (tok->tokType() != Token::eIncDecOp && !tok->isAssignmentOp()) - continue; - if (!tok->astOperand1()) - continue; - for (const Token *tok2 = tok;; tok2 = tok2->astParent()) { - // If ast parent is a sequence point then break - const Token * const parent = tok2->astParent(); - if (!parent) - break; - if (Token::Match(parent, "%oror%|&&|?|:|;")) - break; - if (parent->str() == ",") { - const Token *par = parent; - while (Token::simpleMatch(par,",")) - par = par->astParent(); - // not function or in a while clause => break - if (!(par && par->str() == "(" && par->astOperand2() && par->strAt(-1) != "while")) - break; - // control flow (if|while|etc) => break - if (Token::simpleMatch(par->link(),") {")) - break; - // sequence point in function argument: dostuff((1,2),3) => break - par = par->next(); - while (par && (par->previous() != parent)) - par = par->nextArgument(); - if (!par) - break; - } - if (parent->str() == "(" && parent->astOperand2()) - break; - - // self assignment.. - if (tok2 == tok && - tok->str() == "=" && - parent->str() == "=" && - isSameExpression(false, tok->astOperand1(), parent->astOperand1(), *mSettings, true, false)) { - if (mSettings->severity.isEnabled(Severity::warning) && - isSameExpression(true, tok->astOperand1(), parent->astOperand1(), *mSettings, true, false)) - selfAssignmentError(parent, tok->astOperand1()->expressionString()); - break; - } - - // Is expression used? - bool foundError = false; - visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), - [&](const Token *tok3) { - if (tok3->str() == "&" && !tok3->astOperand2()) - return ChildrenToVisit::none; // don't handle address-of for now - if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof")) - return ChildrenToVisit::none; // don't care about sizeof usage - if (isSameExpression(false, tok->astOperand1(), tok3, *mSettings, true, false)) - foundError = true; - return foundError ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; - }); - - if (foundError) { - unknownEvaluationOrder(parent); - break; - } - } - } + // self assignment.. + if (tok2 == tok && tok->str() == "=" && parent->str() == "=" && isSameExpression(false, tok->astOperand1(), parent->astOperand1(), settings, true, false)) { + if (settings.severity.isEnabled(Severity::warning) && isSameExpression(true, tok->astOperand1(), parent->astOperand1(), settings, true, false)) + selfAssignmentError = true; + return false; } + // Is expression used? + bool foundError = false; + visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) { + if (tok3->str() == "&" && !tok3->astOperand2()) + return ChildrenToVisit::none; // don't handle address-of for now + if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof")) + return ChildrenToVisit::none; // don't care about sizeof usage + if (isSameExpression(false, tok->astOperand1(), tok3, settings, true, false)) + foundError = true; + return foundError ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; + }); + + return foundError; } -void CheckOther::checkEvaluationOrderPost11() +static bool checkEvaluationOrderPost17(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & foundUnspecified) { - // TODO + if (tok->isAssignmentOp()) + return false; + bool foundUndefined{false}; + visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) { + if (tok3->str() == "&" && !tok3->astOperand2()) + return ChildrenToVisit::none; // don't handle address-of for now + if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof")) + return ChildrenToVisit::none; // don't care about sizeof usage + if (isSameExpression(false, tok->astOperand1(), tok3, settings, true, false) && parent->isArithmeticalOp() && parent->isBinaryOp()) + foundUndefined = true; + if (tok3->tokType() == Token::eIncDecOp && isSameExpression(false, tok->astOperand1(), tok3->astOperand1(), settings, true, false)) { + if (parent->isArithmeticalOp() && parent->isBinaryOp()) + foundUndefined = true; + else + foundUnspecified = true; + } + return (foundUndefined || foundUnspecified) ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; + }); + + return foundUndefined || foundUnspecified; } -void CheckOther::checkEvaluationOrderPost17() +void CheckOther::checkEvaluationOrder() { logChecker("CheckOther::checkEvaluationOrder"); const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Scope * functionScope : symbolDatabase->functionScopes) { for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { - if (tok->tokType() != Token::eIncDecOp || !tok->astOperand1()) + if (!tok->isIncDecOp() && !tok->isAssignmentOp()) + continue; + if (!tok->astOperand1()) continue; for (const Token *tok2 = tok;; tok2 = tok2->astParent()) { + // If ast parent is a sequence point then break const Token * const parent = tok2->astParent(); if (!parent) break; @@ -3374,27 +3349,18 @@ void CheckOther::checkEvaluationOrderPost17() if (parent->str() == "(" && parent->astOperand2()) break; - // Is expression used? - bool foundUndefined{false}, foundUnspecified{false}; - visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), - [&](const Token *tok3) { - if (tok3->str() == "&" && !tok3->astOperand2()) - return ChildrenToVisit::none; // don't handle address-of for now - if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof")) - return ChildrenToVisit::none; // don't care about sizeof usage - if (isSameExpression(false, tok->astOperand1(), tok3, *mSettings, true, false) && parent->isArithmeticalOp() && parent->isBinaryOp()) - foundUndefined = true; - if (tok3->tokType() == Token::eIncDecOp && isSameExpression(false, tok->astOperand1(), tok3->astOperand1(), *mSettings, true, false)) { - if (parent->isArithmeticalOp() && parent->isBinaryOp()) - foundUndefined = true; - else - foundUnspecified = true; - } - return (foundUndefined || foundUnspecified) ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; - }); + bool foundError{false}, foundUnspecified{false}, bSelfAssignmentError{false}; + if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP17) + foundError = checkEvaluationOrderPost17(tok, tok2, parent, *mSettings, foundUnspecified); + else + foundError = checkEvaluationOrderPre11(tok, tok2, parent, *mSettings, bSelfAssignmentError); - if (foundUndefined || foundUnspecified) { - unknownEvaluationOrder(parent, !foundUndefined); + if (foundError) { + unknownEvaluationOrder(parent, foundUnspecified); + break; + } + if (bSelfAssignmentError) { + selfAssignmentError(parent, tok->astOperand1()->expressionString()); break; } } @@ -3402,18 +3368,6 @@ void CheckOther::checkEvaluationOrderPost17() } } -void CheckOther::checkEvaluationOrder() -{ - if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11) { - if (mSettings->standards.cpp >= Standards::CPP17) - checkEvaluationOrderPost17(); - else - checkEvaluationOrderPost11(); - } - else - checkEvaluationOrderPre11(); -} - void CheckOther::unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior) { isUnspecifiedBehavior ? diff --git a/lib/checkother.h b/lib/checkother.h index 49651525ef0..f2726bfd4e3 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -212,9 +212,6 @@ class CPPCHECKLIB CheckOther : public Check { /** @brief %Check for expression that depends on order of evaluation of side effects */ void checkEvaluationOrder(); - void checkEvaluationOrderPost17(); - void checkEvaluationOrderPost11(); - void checkEvaluationOrderPre11(); /** @brief %Check for access of moved or forwarded variable */ void checkAccessOfMovedVariable(); From 05e88401a3e69d25045d4e05d23298999fd576f5 Mon Sep 17 00:00:00 2001 From: Oleksandr Labetskyi Date: Wed, 15 May 2024 14:29:41 +0300 Subject: [PATCH 5/6] Fix 2 --- lib/checkother.cpp | 34 +++++++++++++++++++++++++++++----- test/testother.cpp | 29 +++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 813eba71db3..a9c1901901d 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3265,7 +3265,7 @@ void CheckOther::unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef Certainty::normal); } -static bool checkEvaluationOrderPre11(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & selfAssignmentError) +static bool checkEvaluationOrderC(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & selfAssignmentError) { // self assignment.. if (tok2 == tok && tok->str() == "=" && parent->str() == "=" && isSameExpression(false, tok->astOperand1(), parent->astOperand1(), settings, true, false)) { @@ -3288,7 +3288,27 @@ static bool checkEvaluationOrderPre11(const Token * tok, const Token * tok2, con return foundError; } -static bool checkEvaluationOrderPost17(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & foundUnspecified) +static bool checkEvaluationOrderCpp11(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings) +{ + if (tok->isAssignmentOp()) // TODO check assignment + return false; + if (tok->previous() == tok->astOperand1() && parent->isArithmeticalOp() && parent->isBinaryOp()) + return true; + bool foundUndefined{false}; + visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) { + if (tok3->str() == "&" && !tok3->astOperand2()) + return ChildrenToVisit::none; // don't handle address-of for now + if (tok3->str() == "(" && Token::simpleMatch(tok3->previous(), "sizeof")) + return ChildrenToVisit::none; // don't care about sizeof usage + if (isSameExpression(false, tok->astOperand1(), tok3, settings, true, false)) + foundUndefined = true; + return foundUndefined ? ChildrenToVisit::done : ChildrenToVisit::op1_and_op2; + }); + + return foundUndefined; +} + +static bool checkEvaluationOrderCpp17(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & foundUnspecified) { if (tok->isAssignmentOp()) return false; @@ -3350,10 +3370,14 @@ void CheckOther::checkEvaluationOrder() break; bool foundError{false}, foundUnspecified{false}, bSelfAssignmentError{false}; - if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP17) - foundError = checkEvaluationOrderPost17(tok, tok2, parent, *mSettings, foundUnspecified); + if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11) { + if (mSettings->standards.cpp >= Standards::CPP17) + foundError = checkEvaluationOrderCpp17(tok, tok2, parent, *mSettings, foundUnspecified); + else + foundError = checkEvaluationOrderCpp11(tok, tok2, parent, *mSettings); + } else - foundError = checkEvaluationOrderPre11(tok, tok2, parent, *mSettings, bSelfAssignmentError); + foundError = checkEvaluationOrderC(tok, tok2, parent, *mSettings, bSelfAssignmentError); if (foundError) { unknownEvaluationOrder(parent, foundUnspecified); diff --git a/test/testother.cpp b/test/testother.cpp index 52df1238ce5..c16cddd0ec4 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -10799,6 +10799,28 @@ class TestOther : public TestFixture { ASSERT_EQUALS("[test.cpp:6]: (style) Label 'label' is not used.\n", errout_str()); } + #define checkCustomSettings(...) checkCustomSettings_(__FILE__, __LINE__, __VA_ARGS__) + void checkCustomSettings_(const char* file, int line, const char code[], bool cpp = true, bool inconclusive = true, bool runSimpleChecks=true, bool verbose=false, Settings* settings = nullptr) { + if (!settings) { + settings = &_settings; + } + settings->certainty.setEnabled(Certainty::inconclusive, inconclusive); + settings->verbose = verbose; + + // Tokenize.. + SimpleTokenizer tokenizer(*settings, *this); + ASSERT_LOC(tokenizer.tokenize(code, cpp), file, line); + + // Check.. + runChecks(tokenizer, this); + + (void)runSimpleChecks; // TODO Remove this + } + + void checkCustomSettings_(const char* file, int line, const char code[], Settings *s) { + checkCustomSettings_(file, line, code, true, true, true, false, s); + } + void testEvaluationOrder() { check("void f() {\n" " int x = dostuff();\n" @@ -10858,6 +10880,13 @@ class TestOther : public TestFixture { " int n = (~(-(++i)) + i);\n" "}"); ASSERT_EQUALS("[test.cpp:2]: (error) Expression '~(-(++i))+i' depends on order of evaluation of side effects\n", errout_str()); + + /*const*/ Settings settings11 = settingsBuilder(_settings).cpp(Standards::CPP11).build(); + + checkCustomSettings("void f(int i) {\n" + " int n = i++ + 2;\n" + "}", &settings11); + ASSERT_EQUALS("[test.cpp:2]: (error) Expression 'i+++2' depends on order of evaluation of side effects\n", errout_str()); } void testEvaluationOrderSelfAssignment() { From 0804230f09d31d39519b54008ad4a05ba9442606 Mon Sep 17 00:00:00 2001 From: Oleksandr Labetskyi Date: Thu, 16 May 2024 15:22:05 +0300 Subject: [PATCH 6/6] Fix --- lib/checkother.cpp | 6 ++++-- test/testother.cpp | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index a9c1901901d..2ec02e038ed 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3292,8 +3292,10 @@ static bool checkEvaluationOrderCpp11(const Token * tok, const Token * tok2, con { if (tok->isAssignmentOp()) // TODO check assignment return false; - if (tok->previous() == tok->astOperand1() && parent->isArithmeticalOp() && parent->isBinaryOp()) - return true; + if (tok->previous() == tok->astOperand1() && parent->isArithmeticalOp() && parent->isBinaryOp()) { + if (parent->astParent() && parent->astParent()->isAssignmentOp() && isSameExpression(false, tok->astOperand1(), parent->astParent()->astOperand1(), settings, true, false)) + return true; + } bool foundUndefined{false}; visitAstNodes((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2(), [&](const Token *tok3) { if (tok3->str() == "&" && !tok3->astOperand2()) diff --git a/test/testother.cpp b/test/testother.cpp index c16cddd0ec4..4b2d60a8881 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -10884,7 +10884,7 @@ class TestOther : public TestFixture { /*const*/ Settings settings11 = settingsBuilder(_settings).cpp(Standards::CPP11).build(); checkCustomSettings("void f(int i) {\n" - " int n = i++ + 2;\n" + " i = i++ + 2;\n" "}", &settings11); ASSERT_EQUALS("[test.cpp:2]: (error) Expression 'i+++2' depends on order of evaluation of side effects\n", errout_str()); }