Fixed #3206 and #7226 (New check: Undefined execution order)

lib/checkother.cpp

@@ -2481,3 +2481,59 @@ void CheckOther::unusedLabelError(const Token* tok)
     reportError(tok, Severity::style, "unusedLabel",
                 "Label '" + (tok?tok->str():emptyString) + "' is not used.");
 }
+
+
+void CheckOther::checkEvaluationOrder()
+{
+    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
+    const std::size_t functions = symbolDatabase->functionScopes.size();
+    for (std::size_t i = 0; i < functions; ++i) {
+        const Scope * functionScope = symbolDatabase->functionScopes[i];
+        for (const Token* tok = functionScope->classStart; tok != functionScope->classEnd; tok = tok->next()) {
+            if (!Token::Match(tok, "++|--") && !tok->isAssignmentOp())
+                continue;
+            if (!tok->astOperand1())
+                continue;
+            for (const Token *tok2 = tok; tok2 && tok2->astParent(); tok2 = tok2->astParent()) {
+                const Token * const parent = tok2->astParent();
+                if (Token::Match(parent, "%oror%|&&|?|:|;"))
+                    break;
+                if (parent->str() == ",") {
+                    const Token *par = parent;
+                    while (Token::simpleMatch(par,","))
+                        par = par->astParent();
+                    if (!par || par->str() != "(")
+                        break;
+                }
+
+                // Is expression used?
+                bool foundError = false;
+                std::stack<const Token *> tokens;
+                tokens.push((parent->astOperand1() != tok2) ? parent->astOperand1() : parent->astOperand2());
+                while (!tokens.empty() && !foundError) {
+                    const Token * const tok3 = tokens.top();
+                    tokens.pop();
+                    if (!tok3)
+                        continue;
+                    tokens.push(tok3->astOperand1());
+                    tokens.push(tok3->astOperand2());
+                    if (isSameExpression(_tokenizer->isCPP(), tok->astOperand1(), tok3, _settings->library.functionpure)) {
+                        foundError = true;
+                    }
+                }
+
+                if (foundError) {
+                    unknownEvaluationOrder(parent);
+                    break;
+                }
+            }
+        }
+    }
+}
+
+void CheckOther::unknownEvaluationOrder(const Token* tok)
+{
+    reportError(tok, Severity::error, "unknownEvaluationOrder",
+                "Expression '" + (tok ? tok->expressionString() : std::string("x = x++;")) + "' depends on order of evaluation of side effects");
+}
+

lib/checkother.h

@@ -70,6 +70,7 @@ public:
         checkOther.checkZeroDivision();
         checkOther.checkInterlockedDecrement();
         checkOther.checkUnusedLabel();
+        checkOther.checkEvaluationOrder();
     }
 
     /** @brief Run checks against the simplified token list */
@@ -203,6 +204,9 @@ public:
     /** @brief %Check for unused labels */
     void checkUnusedLabel();
 
+    /** @brief %Check for expression that depends on order of evaluation of side effects */
+    void checkEvaluationOrder();
+
 private:
     // Error messages..
     void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result);
@@ -251,6 +255,7 @@ private:
     void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive);
     void raceAfterInterlockedDecrementError(const Token* tok);
     void unusedLabelError(const Token* tok);
+    void unknownEvaluationOrder(const Token* tok);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckOther c(0, settings, errorLogger);
@@ -305,6 +310,7 @@ private:
         c.commaSeparatedReturnError(0);
         c.redundantPointerOpError(0, "varname", false);
         c.unusedLabelError(0);
+        c.unknownEvaluationOrder(0);
     }
 
     static std::string myName() {
@@ -323,6 +329,7 @@ private:
                "- provide wrong dimensioned array to pipe() system command (--std=posix)\n"
                "- cast the return values of getc(),fgetc() and getchar() to character and compare it to EOF\n"
                "- race condition with non-interlocked access after InterlockedDecrement() call\n"
+               "- expression 'x = x++;' depends on order of evaluation of side effects\n"
 
                // warning
                "- either division by zero or useless condition\n"

lib/token.cpp

@@ -1142,7 +1142,8 @@ std::string Token::expressionString() const
         start = start->astOperand1();
     const Token *end = top;
     while (end->astOperand1() && (end->astOperand2() || end->isUnaryPreOp())) {
-        if (Token::Match(end,"(|[")) {
+        if (Token::Match(end,"(|[") &&
+            !(Token::Match(end, "( %type%") && !end->astOperand2())) {
             end = end->link();
             break;
         }

test/testother.cpp

@@ -157,6 +157,8 @@ private:
         TEST_CASE(raceAfterInterlockedDecrement);
 
         TEST_CASE(testUnusedLabel);
+
+        TEST_CASE(testEvaluationOrder);
     }
 
     void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) {
@@ -2884,12 +2886,6 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'x' to itself.\n", errout.str());
 
-        check("void foo()\n"
-              "{\n"
-              "    std::string var = var = \"test\";\n"
-              "}");
-        TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'var' to itself.\n", "", errout.str());
-
         check("struct A { int b; };\n"
               "void foo(A* a1, A* a2) {\n"
               "    a1->b = a1->b;\n"
@@ -6102,6 +6098,20 @@ private:
               "}");
         ASSERT_EQUALS("", errout.str());
     }
+
+    void testEvaluationOrder() {
+        check("void f() {\n"
+              "  int x = dostuff();\n"
+              "  return x + x++;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Expression 'x+x++' depends on order of evaluation of side effects\n", errout.str());
+
+        // #7226
+        check("long int f1(const char *exp) {\n"
+              "  return strtol(++exp, (char **)&exp, 10);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (error) Expression '++exp,(char**)&exp' depends on order of evaluation of side effects\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)