Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1509273005:
Adds additional tests for bytecode graph builder (Closed)
DescriptionAdds additional tests for bytecode graph builder
Adds more tests for Delete, InstanceOf, and ToName bytecodes.
BUG=v8:4280
LOG=N
Committed: https://crrev.com/67f3c80da9a065837f9de187069b955d1281c9b7
Cr-Commit-Position: refs/heads/master@{#32763}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixed comments. Added more tests for ToName bytecode. #
Total comments: 6
Patch Set 3 : Added tests for ToName to interpreter. #
Messages
Total messages: 17 (6 generated)
mythria@chromium.org changed reviewers: + mstarzinger@chromium.org, oth@chromium.org, rmcilroy@chromium.org
I fixed todos in test-run-bytecode-graph-builder. Could you please review my changes. Thanks, Mythri https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:728: {factory->NewNumberFromInt(10)}}, Is this test Ok? I will remove it if ToName of an object does not consistently return the same string.
Description was changed from ========== Adds additional tests for bytecode graph builder Adds more tests for Delete, InstanceOf, and ToName bytecodes. BUG=V8:4280 LOG=N ========== to ========== Adds additional tests for bytecode graph builder Adds more tests for Delete, InstanceOf, and ToName bytecodes. BUG=v8:4280 LOG=N ==========
LGTM with nits and suggestions. https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:711: TEST(BytecodeGraphBuilderCast) { nit: We could rename this test to something like "BytecodeGraphBuilderCastToName" because currently only ToName is tested in here. Additional tests (e.g. ToObject) could then be separated out for readability. WDYT? https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:728: {factory->NewNumberFromInt(10)}}, On 2015/12/09 14:14:54, mythria wrote: > Is this test Ok? I will remove it if ToName of an object does not consistently > return the same string. Acknowledged. Should be fine as it is. You could add "var a = {toString: function(){ return 'x' }}" as well to test the most generic case of ToName as well. And if you really want to go fancy you can also check "var a = {valueOf: function() { return 'x' }}" which should not get called, IIRC. Also "var a = {[Symbol.toPrimitive]: function() { return 'x' }}" for the some ES6 coverage. https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:1115: {"var cons = function() { this.val = 10; this.type = 'int'; };" nit: Properties are never used, empty function would do the trick, right?
Thanks for the new tests. I addressed the comments. Thanks, Mythri https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:711: TEST(BytecodeGraphBuilderCast) { On 2015/12/09 16:12:48, Michael Starzinger wrote: > nit: We could rename this test to something like > "BytecodeGraphBuilderCastToName" because currently only ToName is tested in > here. Additional tests (e.g. ToObject) could then be separated out for > readability. WDYT? Done. https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:728: {factory->NewNumberFromInt(10)}}, On 2015/12/09 16:12:49, Michael Starzinger wrote: > On 2015/12/09 14:14:54, mythria wrote: > > Is this test Ok? I will remove it if ToName of an object does not consistently > > return the same string. > > Acknowledged. Should be fine as it is. > > You could add "var a = {toString: function(){ return 'x' }}" as well to test the > most generic case of ToName as well. > > And if you really want to go fancy you can also check "var a = {valueOf: > function() { return 'x' }}" which should not get called, IIRC. > > Also "var a = {[Symbol.toPrimitive]: function() { return 'x' }}" for the some > ES6 coverage. Thanks for the tests. Done. https://codereview.chromium.org/1509273005/diff/1/test/cctest/compiler/test-r... test/cctest/compiler/test-run-bytecode-graph-builder.cc:1115: {"var cons = function() { this.val = 10; this.type = 'int'; };" On 2015/12/09 16:12:48, Michael Starzinger wrote: > nit: Properties are never used, empty function would do the trick, right? Done.
https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:204: {"return NaN;", {factory->NewNumber(NAN)}}}; If this is giving you trouble, just use Factory::nan_value instead.
lgtm once the NAN issue is fixed. Thanks for the new tests! https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:747: "return obj.x;", If you could add these new tests to test-interpreter too that would be awesome. https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:1139: {"return typeof p1;", Should the typeof tests be a separate test?
Thanks, the nan issue is fixed now. All trybots succeed. https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... File test/cctest/compiler/test-run-bytecode-graph-builder.cc (right): https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:204: {"return NaN;", {factory->NewNumber(NAN)}}}; On 2015/12/09 17:29:53, Michael Starzinger wrote: > If this is giving you trouble, just use Factory::nan_value instead. Thanks :). Done. https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:747: "return obj.x;", On 2015/12/10 11:39:30, rmcilroy wrote: > If you could add these new tests to test-interpreter too that would be awesome. Done. https://codereview.chromium.org/1509273005/diff/20001/test/cctest/compiler/te... test/cctest/compiler/test-run-bytecode-graph-builder.cc:1139: {"return typeof p1;", On 2015/12/10 11:39:30, rmcilroy wrote: > Should the typeof tests be a separate test? oops, copy-paste error. These tests are already a part of a seperate typeof test. Forgot to delete them.
lgtm, thanks.
awesome, lgtm.
The CQ bit was checked by mythria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstarzinger@chromium.org Link to the patchset: https://codereview.chromium.org/1509273005/#ps40001 (title: "Added tests for ToName to interpreter.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1509273005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1509273005/40001
Message was sent while issue was closed.
Description was changed from ========== Adds additional tests for bytecode graph builder Adds more tests for Delete, InstanceOf, and ToName bytecodes. BUG=v8:4280 LOG=N ========== to ========== Adds additional tests for bytecode graph builder Adds more tests for Delete, InstanceOf, and ToName bytecodes. BUG=v8:4280 LOG=N ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Adds additional tests for bytecode graph builder Adds more tests for Delete, InstanceOf, and ToName bytecodes. BUG=v8:4280 LOG=N ========== to ========== Adds additional tests for bytecode graph builder Adds more tests for Delete, InstanceOf, and ToName bytecodes. BUG=v8:4280 LOG=N Committed: https://crrev.com/67f3c80da9a065837f9de187069b955d1281c9b7 Cr-Commit-Position: refs/heads/master@{#32763} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/67f3c80da9a065837f9de187069b955d1281c9b7 Cr-Commit-Position: refs/heads/master@{#32763} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
