ValueFlow: improved valueflow for loops that assign variable and then break

danmar · danmar · commit ee93d30d94ec · 2014-09-23T16:06:02.000+02:00
diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
@@ -820,9 +820,34 @@ static bool valueFlowForward(Token * const               startToken,
                     ++number_of_if;
                     tok2 = end;
                 } else {
-                    if (settings->debugwarnings)
-                        bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " is assigned in conditional code");
-                    return false;
+                    bool bail = true;
+
+                    // loop that conditionally set variable and then break => either loop condition is
+                    // redundant or the variable can be unchanged after the loop.
+                    bool loopCondition = false;
+                    if (Token::simpleMatch(tok2, "while (") && Token::Match(tok2->next()->astOperand2(), "%op%"))
+                        loopCondition = true;
+                    else if (Token::simpleMatch(tok2, "for (") &&
+                             Token::simpleMatch(tok2->next()->astOperand2(), ";") &&
+                             Token::simpleMatch(tok2->next()->astOperand2()->astOperand2(), ";") &&
+                             Token::Match(tok2->next()->astOperand2()->astOperand2()->astOperand1(), "%op%"))
+                        loopCondition = true;
+                    if (loopCondition) {
+                        const Token *tok3 = Token::findmatch(start, "%varid%", end, varid);
+                        if (Token::Match(tok3, "%varid% =", varid) &&
+                            tok3->scope()->classEnd                &&
+                            Token::Match(tok3->scope()->classEnd->tokAt(-3), "[;}] break ;") &&
+                            !Token::findmatch(tok3->next(), "%varid%", end, varid)) {
+                            bail = false;
+                            tok2 = end;
+                        }
+                    }
+
+                    if (bail) {
+                        if (settings->debugwarnings)
+                            bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " is assigned in conditional code");
+                        return false;
+                    }
                 }
             }
         }
diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp
@@ -1099,7 +1099,7 @@ class TestNullPointer : public TestFixture {
               "\n"
               "    *p = 0;\n"
               "}");
-        TODO_ASSERT_EQUALS("[test.cpp:11]: (error) Possible null pointer dereference: p\n", "", errout.str());
+        ASSERT_EQUALS("[test.cpp:11]: (error) Possible null pointer dereference: p\n", errout.str());
     }
 
     void nullpointer7() {
diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp
@@ -871,6 +871,43 @@ class TestValueFlow : public TestFixture {
                "  }\n"
                "}";
         ASSERT_EQUALS(false, testValueOfX(code, 8U, 34));
+
+        // while/for
+        code = "void f(const int *buf) {\n"
+               "  int x = 0;\n"
+               "  for (int i = 0; i < 10; i++) {\n"
+               "    if (buf[i] == 123) {\n"
+               "      x = i;\n"
+               "      break;\n"
+               "    }\n"
+               "  }\n"
+               "  a = x;\n" // <- x can be 0
+               "}\n";
+        ASSERT_EQUALS(true, testValueOfX(code, 9U, 0)); // x can be 0 at line 9
+
+        code = "void f(const int *buf) {\n"
+               "  int x = 0;\n"
+               "  for (int i = 0; i < 10; i++) {\n"
+               "    if (buf[i] == 123) {\n"
+               "      x = i;\n"
+               "      ;\n" // <- no break
+               "    }\n"
+               "  }\n"
+               "  a = x;\n" // <- x cant be 0
+               "}\n";
+        ASSERT_EQUALS(false, testValueOfX(code, 9U, 0)); // x cant be 0 at line 9
+
+        code = "void f(const int *buf) {\n"
+               "  int x = 0;\n"
+               "  while (++i < 10) {\n"
+               "    if (buf[i] == 123) {\n"
+               "      x = i;\n"
+               "      break;\n"
+               "    }\n"
+               "  }\n"
+               "  a = x;\n" // <- x can be 0
+               "}\n";
+        ASSERT_EQUALS(true, testValueOfX(code, 9U, 0)); // x can be 0 at line 9
     }
 
     void valueFlowAfterCondition() {

Original file line number	Diff line number	Diff line change
`@@ -1099,7 +1099,7 @@ class TestNullPointer : public TestFixture {`
`1099`	`1099`	`"\n"`
`1100`	`1100`	`" *p = 0;\n"`
`1101`	`1101`	`"}");`
`1102`		`- TODO_ASSERT_EQUALS("[test.cpp:11]: (error) Possible null pointer dereference: p\n", "", errout.str());`
	`1102`	`+ ASSERT_EQUALS("[test.cpp:11]: (error) Possible null pointer dereference: p\n", errout.str());`
`1103`	`1103`	`}`
`1104`	`1104`
`1105`	`1105`	`void nullpointer7() {`