Addressed feedback.
http://codereview.chromium.org/9141016/diff/29002/test/mjsunit/elements-transition-hoisting.js File test/mjsunit/elements-transition-hoisting.js (right): http://codereview.chromium.org/9141016/diff/29002/test/mjsunit/elements-transition-hoisting.js#newcode45 test/mjsunit/elements-transition-hoisting.js:45: // Make sure that multiple stores to an object array get hoisted in a way that On 2012/02/07 10:05:05, fschneider wrote:
What is meant by hoisting multiple stores? Stores themselves never get
hoisted. Done. http://codereview.chromium.org/9141016/diff/29002/test/mjsunit/elements-transition-hoisting.js#newcode52 test/mjsunit/elements-transition-hoisting.js:52: } while (global_false); On 2012/02/07 10:05:05, fschneider wrote:
Maybe you should go around the loop >1 times in all tests where you
want to test
that hoisting does not cause deopts.
Done. http://codereview.chromium.org/9141016/diff/29002/test/mjsunit/elements-transition-hoisting.js#newcode101 test/mjsunit/elements-transition-hoisting.js:101: // Make sure that non-element related map checks still get hoisted if all On 2012/02/07 10:05:05, fschneider wrote:
The description of this test does not match the comment below where
you write
that the map check should not be hoisted.
Done. http://codereview.chromium.org/9141016/diff/29002/test/mjsunit/elements-transition-hoisting.js#newcode111 test/mjsunit/elements-transition-hoisting.js:111: a.foo = object; // This map check should NOT be hoistable because includes On 2012/02/07 10:05:05, fschneider wrote:
When running this test I see this map check hoisted which contradicts
your
expectation here.
Done. http://codereview.chromium.org/9141016/diff/29002/test/mjsunit/elements-transition-hoisting.js#newcode128 test/mjsunit/elements-transition-hoisting.js:128: a.foo = 0; On 2012/02/07 10:05:05, fschneider wrote:
Since a.foo is not used in the loop, I don't see why it's necessary to
add it
here.
Done. http://codereview.chromium.org/9141016/diff/29002/test/mjsunit/elements-transition-hoisting.js#newcode131 test/mjsunit/elements-transition-hoisting.js:131: if (global_false != true) { On 2012/02/07 10:05:05, fschneider wrote:
if (!global_false)
Otherwise the compare is generic (and a call inside the loop).
Done. http://codereview.chromium.org/9141016/diff/29002/test/mjsunit/elements-transition-hoisting.js#newcode146 test/mjsunit/elements-transition-hoisting.js:146: a.foo = 0; On 2012/02/07 10:05:05, fschneider wrote:
Since a.foo is not used in the loop, I don't see why it's necessary to
add it
here.
Done. http://codereview.chromium.org/9141016/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
