Fix #12726: Update for CheckOrderEvaluation

lib/checkother.cpp

@@ -3265,19 +3265,82 @@ void CheckOther::unusedLabelError(const Token* tok, bool inSwitch, bool hasIfdef
                 Certainty::normal);
 }
 
+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)) {
+        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;
+    });
 
-void CheckOther::checkEvaluationOrder()
+    return foundError;
+}
+
+static bool checkEvaluationOrderCpp11(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings)
 {
-    // This checker is not written according to C++11 sequencing rules
-    if (mTokenizer->isCPP() && mSettings->standards.cpp >= Standards::CPP11)
-        return;
+    if (tok->isAssignmentOp()) // TODO check assignment
+        return false;
+    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())
+            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;
+    });
 
-    logChecker("CheckOther::checkEvaluationOrder"); // C/C++03
+    return foundUndefined;
+}
 
+static bool checkEvaluationOrderCpp17(const Token * tok, const Token * tok2, const Token * parent, const Settings & settings, bool & foundUnspecified)
+{
+    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::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->isAssignmentOp())
+            if (!tok->isIncDecOp() && !tok->isAssignmentOp())
                 continue;
             if (!tok->astOperand1())
                 continue;
@@ -3308,43 +3371,36 @@ void CheckOther::checkEvaluationOrder()
                 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;
+                bool foundError{false}, foundUnspecified{false}, bSelfAssignmentError{false};
+                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);
                 }
-
-                // 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;
-                });
+                else
+                    foundError = checkEvaluationOrderC(tok, tok2, parent, *mSettings, bSelfAssignmentError);
 
                 if (foundError) {
-                    unknownEvaluationOrder(parent);
+                    unknownEvaluationOrder(parent, foundUnspecified);
+                    break;
+                }
+                if (bSelfAssignmentError) {
+                    selfAssignmentError(parent, tok->astOperand1()->expressionString());
                     break;
                 }
             }
         }
     }
 }
 
-void CheckOther::unknownEvaluationOrder(const Token* tok)
+void CheckOther::unknownEvaluationOrder(const Token* tok, bool isUnspecifiedBehavior)
 {
-    reportError(tok, 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()

lib/checkother.h

@@ -280,7 +280,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<const Token*> & declarations, const std::vector<const Token*> & definitions);

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<CheckOther>(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"
@@ -10842,6 +10864,29 @@ 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. 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());
+
+        /*const*/ Settings settings11 = settingsBuilder(_settings).cpp(Standards::CPP11).build();
+
+        checkCustomSettings("void f(int i) {\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());
     }
 
     void testEvaluationOrderSelfAssignment() {