Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2761673003:
[typedarrays] Use internal byteOffset in TypedArray.prototype.set (Closed)
|
Created:
3 years, 9 months ago by Choongwoo Han Modified:
3 years, 8 months ago Reviewers:
Dan Ehrenberg CC:
v8-reviews_googlegroups.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
DescriptionUse internal byteOffset in TypedArray.prototype.set
Since byteOffset is configurable, we need to access byteOffset by
%_ArrayBufferViewGetByteOffset, instead of accessing .byteOffset
property.
BUG=v8:6120
Review-Url: https://codereview.chromium.org/2761673003
Cr-Commit-Position: refs/heads/master@{#44347}
Committed: https://chromium.googlesource.com/v8/v8/+/8c2af0379130b31bc244ed811760553dfbd307a0
Patch Set 1 #
Total comments: 2
Patch Set 2 : make test for test262 #
Total comments: 2
Patch Set 3 : copyright 2017 #Patch Set 4 : path #
Messages
Total messages: 31 (21 generated)
Description was changed from ========== [typedarrays] Use internal byteOffset in TypedArray.prototype.set Since byteOffset is configurable, we need to access byteOffset by %_ArrayBufferViewGetByteOffset, instead of accessing .byteOffset property. BUG=v8:6120 ========== to ========== [typedarrays] Use internal byteOffset in TypedArray.prototype.set Since byteOffset is configurable, we need to access byteOffset by %_ArrayBufferViewGetByteOffset, instead of accessing .byteOffset property. BUG=v8:6120 ==========
cwhan.tunz@gmail.com changed reviewers: + littledan@chromium.org
PTAL
Description was changed from ========== [typedarrays] Use internal byteOffset in TypedArray.prototype.set Since byteOffset is configurable, we need to access byteOffset by %_ArrayBufferViewGetByteOffset, instead of accessing .byteOffset property. BUG=v8:6120 ========== to ========== Use internal byteOffset in TypedArray.prototype.set Since byteOffset is configurable, we need to access byteOffset by %_ArrayBufferViewGetByteOffset, instead of accessing .byteOffset property. BUG=v8:6120 ==========
littledan@, Can you take a look?
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Thanks for the fix! https://codereview.chromium.org/2761673003/diff/1/test/mjsunit/es6/typedarray... File test/mjsunit/es6/typedarray-set-byteoffset-internal.js (right): https://codereview.chromium.org/2761673003/diff/1/test/mjsunit/es6/typedarray... test/mjsunit/es6/typedarray-set-byteoffset-internal.js:35: } These tests are great. Nit: Please cross-reference the bug that this is a regression test for, either in a comment in this test file, or by naming it test/mjsunit/regress-6120.js. What would be even greater is upstream test262 tests so that other JS engine vendors can also run them. You can put a test in the directory test/test262/local-test ; find more information at https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5... if you want to write the test in this patch. Or, if you want to just check in this version of the test, you could contribute a separate upstream test262 test directly at https://github.com/tc39/test262/ . In the description field, you can cross-reference the V8 bug.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2761673003/diff/1/test/mjsunit/es6/typedarray... File test/mjsunit/es6/typedarray-set-byteoffset-internal.js (right): https://codereview.chromium.org/2761673003/diff/1/test/mjsunit/es6/typedarray... test/mjsunit/es6/typedarray-set-byteoffset-internal.js:35: } On 2017/04/03 09:39:24, Dan Ehrenberg wrote: > These tests are great. Nit: Please cross-reference the bug that this is a > regression test for, either in a comment in this test file, or by naming it > test/mjsunit/regress-6120.js. > > What would be even greater is upstream test262 tests so that other JS engine > vendors can also run them. You can put a test in the directory > test/test262/local-test ; find more information at > https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5... > if you want to write the test in this patch. Or, if you want to just check in > this version of the test, you could contribute a separate upstream test262 test > directly at https://github.com/tc39/test262/ . In the description field, you can > cross-reference the V8 bug. I cannot fully understand the process of the test262. Is the following my understanding right? 1. put the test only into test/test262/local-test (not in mjsunit) 2. land this CL 3. make a PR to upstream 4. remove local-test (with another CL?) when it is merged into upstream
On 2017/04/03 11:43:38, Choongwoo Han wrote: > https://codereview.chromium.org/2761673003/diff/1/test/mjsunit/es6/typedarray... > File test/mjsunit/es6/typedarray-set-byteoffset-internal.js (right): > > https://codereview.chromium.org/2761673003/diff/1/test/mjsunit/es6/typedarray... > test/mjsunit/es6/typedarray-set-byteoffset-internal.js:35: } > On 2017/04/03 09:39:24, Dan Ehrenberg wrote: > > These tests are great. Nit: Please cross-reference the bug that this is a > > regression test for, either in a comment in this test file, or by naming it > > test/mjsunit/regress-6120.js. > > > > What would be even greater is upstream test262 tests so that other JS engine > > vendors can also run them. You can put a test in the directory > > test/test262/local-test ; find more information at > > > https://docs.google.com/document/d/16bj7AIDgZLv4WOsUEzQ5NzcEN9_xo095e88Pz8FC5... > > if you want to write the test in this patch. Or, if you want to just check in > > this version of the test, you could contribute a separate upstream test262 > test > > directly at https://github.com/tc39/test262/ . In the description field, you > can > > cross-reference the V8 bug. > > I cannot fully understand the process of the test262. Is the following my > understanding right? > 1. put the test only into test/test262/local-test (not in mjsunit) > 2. land this CL > 3. make a PR to upstream > 4. remove local-test (with another CL?) when it is merged into upstream Step 4 there is my job. The script upstream-local-tests.sh is supposed to help you with step 3. If you find it easier, it would be fine to just do the upstream tests, since you already wrote mjsunit tests here.
The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Pushed new tests for test262. Can you take another look?
lgtm Looks great! Thanks for taking the time to redo it this way. https://codereview.chromium.org/2761673003/diff/20001/test/test262/local-test... File test/test262/local-tests/test/TypedArray/prototype/set/typedarray-arg-src-byteoffset-internal.js (right): https://codereview.chromium.org/2761673003/diff/20001/test/test262/local-test... test/test262/local-tests/test/TypedArray/prototype/set/typedarray-arg-src-byteoffset-internal.js:1: // Copyright 2016 the V8 project authors. All rights reserved. 2017?
thanks https://codereview.chromium.org/2761673003/diff/20001/test/test262/local-test... File test/test262/local-tests/test/TypedArray/prototype/set/typedarray-arg-src-byteoffset-internal.js (right): https://codereview.chromium.org/2761673003/diff/20001/test/test262/local-test... test/test262/local-tests/test/TypedArray/prototype/set/typedarray-arg-src-byteoffset-internal.js:1: // Copyright 2016 the V8 project authors. All rights reserved. On 2017/04/03 14:37:27, Dan Ehrenberg wrote: > 2017? Done.
The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by cwhan.tunz@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cwhan.tunz@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from littledan@chromium.org Link to the patchset: https://codereview.chromium.org/2761673003/#ps60001 (title: "path")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491232794072170,
"parent_rev": "7a3a1eec12c9305322dc56ea72e9f462fc175f3b", "commit_rev":
"8c2af0379130b31bc244ed811760553dfbd307a0"}
Message was sent while issue was closed.
Description was changed from ========== Use internal byteOffset in TypedArray.prototype.set Since byteOffset is configurable, we need to access byteOffset by %_ArrayBufferViewGetByteOffset, instead of accessing .byteOffset property. BUG=v8:6120 ========== to ========== Use internal byteOffset in TypedArray.prototype.set Since byteOffset is configurable, we need to access byteOffset by %_ArrayBufferViewGetByteOffset, instead of accessing .byteOffset property. BUG=v8:6120 Review-Url: https://codereview.chromium.org/2761673003 Cr-Commit-Position: refs/heads/master@{#44347} Committed: https://chromium.googlesource.com/v8/v8/+/8c2af0379130b31bc244ed811760553dfbd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/8c2af0379130b31bc244ed811760553dfbd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
