Don't evaluate the expression in switch() multiple times

The old code would evaluate the expression in the switch
statement once for every case label. This is not only slower
than it should be, but can also lead to unexpected results in
case the expression doesn't always evaluate to the same value
or has side effects.

Task-number: QTBUG-41630
Change-Id: Id93baca7e3aa09ce884967ef6524d4c4f055bcd6
Reviewed-by: Simon Hausmann <simon.hausmann@theqtcompany.com>

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index 27aecdd..02274ca 100644 (file)
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -2448,7 +2448,8 @@ bool Codegen::visit(SwitchStatement *ast)
     IR::BasicBlock *switchend = _function->newBasicBlock(exceptionHandler());
 
     if (ast->block) {
-        Result lhs = expression(ast->expression);
+        int lhs = _block->newTemp();
+        move(_block->TEMP(lhs), *expression(ast->expression));
         IR::BasicBlock *switchcond = _function->newBasicBlock(exceptionHandler());
         _block->JUMP(switchcond);
         IR::BasicBlock *previousBlock = 0;
@@ -2510,7 +2511,7 @@ bool Codegen::visit(SwitchStatement *ast)
             Result rhs = expression(clause->expression);
             IR::BasicBlock *iftrue = blockMap[clause];
             IR::BasicBlock *iffalse = _function->newBasicBlock(exceptionHandler());
-            setLocation(cjump(binop(IR::OpStrictEqual, *lhs, *rhs), iftrue, iffalse), clause->caseToken);
+            setLocation(cjump(binop(IR::OpStrictEqual, _block->TEMP(lhs), *rhs), iftrue, iffalse), clause->caseToken);
             _block = iffalse;
         }
 
@@ -2519,7 +2520,7 @@ bool Codegen::visit(SwitchStatement *ast)
             Result rhs = expression(clause->expression);
             IR::BasicBlock *iftrue = blockMap[clause];
             IR::BasicBlock *iffalse = _function->newBasicBlock(exceptionHandler());
-            setLocation(cjump(binop(IR::OpStrictEqual, *lhs, *rhs), iftrue, iffalse), clause->caseToken);
+            setLocation(cjump(binop(IR::OpStrictEqual, _block->TEMP(lhs), *rhs), iftrue, iffalse), clause->caseToken);
             _block = iffalse;
         }
 

diff --git a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
index 722fd19..def0476 100644 (file)
--- a/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
+++ b/tests/auto/qml/qqmlecmascript/tst_qqmlecmascript.cpp
@@ -325,6 +325,7 @@ private slots:
     void qtbug_39520();
     void readUnregisteredQObjectProperty();
     void writeUnregisteredQObjectProperty();
+    void switchExpression();
 
 private:
 //    static void propertyVarWeakRefCallback(v8::Persistent<v8::Value> object, void* parameter);
@@ -7843,6 +7844,29 @@ void tst_qqmlecmascript::writeUnregisteredQObjectProperty()
     QCOMPARE(root->property("container").value<ObjectContainer*>()->mSetterCalled, true);
 }
 
+void tst_qqmlecmascript::switchExpression()
+{
+    // verify that we evaluate the expression inside switch() exactly once
+    QJSEngine engine;
+    QJSValue v = engine.evaluate(QString::fromLatin1(
+            "var num = 0\n"
+            "var x = 0\n"
+            "function f() { ++num; return (Math.random() > 0.5) ? 0 : 1; }\n"
+            "for (var i = 0; i < 1000; ++i) {\n"
+            "   switch (f()) {\n"
+            "   case 0:\n"
+            "   case 1:\n"
+            "       break;\n"
+            "   default:\n"
+            "       ++x;\n"
+            "   }\n"
+            "}\n"
+            "(x == 0 && num == 1000) ? true : false\n"
+                        ));
+    QVERIFY(!v.isError());
+    QCOMPARE(v.toBool(), true);
+}
+
 QTEST_MAIN(tst_qqmlecmascript)
 
 #include "tst_qqmlecmascript.moc"