Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1427573005:
[es6] Implement @@split subclassing. (Closed)
|
Created:
5 years, 1 month ago by Yang Modified:
5 years, 1 month ago Reviewers:
Dan Ehrenberg CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[es6] Implement @@split subclassing.
RegExp.prototye[@@split] is not yet implement to spec regarding creating
new RegExp object with the SpeciesConstructor.
R=littledan@chromium.org
BUG=v8:4345
LOG=N
Committed: https://crrev.com/9a569ec2c818e17fd2286459202b9fb5ba3d27af
Cr-Commit-Position: refs/heads/master@{#31911}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix #Patch Set 4 : rebase #Patch Set 5 : re-rebase #Patch Set 6 : update webkit test expectation #Patch Set 7 : use TO_UINT32 #Patch Set 8 : add comments #Patch Set 9 : move test case #Patch Set 10 : added test for name #
Total comments: 1
Patch Set 11 : use kMaxUint32 #
Messages
Total messages: 40 (18 generated)
Description was changed from ========== [es6] Implement @@split subclassing. RegExp[@@split] is not yet implement to spec regarding creating new RegExp object with the SpeciesConstructor. R=littledan@chromium.org BUG=v8:4345 LOG=N ========== to ========== [es6] Implement @@split subclassing. RegExp.prototye[@@split] is not yet implement to spec regarding creating new RegExp object with the SpeciesConstructor. R=littledan@chromium.org BUG=v8:4345 LOG=N ==========
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...) v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/7390)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/7410)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...) v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/9816) v8_linux_dbg on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg/builds/10432) v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_mac_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/11529)
On 2015/11/04 20:10:10, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > v8_mac_rel on tryserver.v8 (JOB_FAILED, > http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/11529) Dan, the failing test is this: FAIL 'test'.split(/(?:)/, -1) should be t,e,s,t. Was . Given that RegExp.prototype[@@split] applies ToLength (and no longer ToUint32) on the limit argument, -1 should turn into 0, instead of 4294967295, right? So the test failure is working as intended?
On 2015/11/04 20:20:07, Yang wrote: > On 2015/11/04 20:10:10, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > v8_mac_rel on tryserver.v8 (JOB_FAILED, > > http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/11529) > > Dan, the failing test is this: > > FAIL 'test'.split(/(?:)/, -1) should be t,e,s,t. Was . > > Given that RegExp.prototype[@@split] applies ToLength (and no longer ToUint32) > on the limit argument, -1 should turn into 0, instead of 4294967295, right? So > the test failure is working as intended? There's actually a spec bug open to change this back to the ES5 definition: https://github.com/tc39/ecma262/issues/92 (suggested by Andre Bargull after we did the same thing for String in https://github.com/tc39/ecma262/pull/86)
The CQ bit was checked by yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/100001
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 yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/120001
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 yangguo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/140001
lgtm Looks correct to me, modulo issues that we've already discussed. https://codereview.chromium.org/1427573005/diff/180001/src/js/regexp.js File src/js/regexp.js (right): https://codereview.chromium.org/1427573005/diff/180001/src/js/regexp.js#newco... src/js/regexp.js:289: limit = (IS_UNDEFINED(limit)) ? 0xffffffff : TO_UINT32(limit); I added a kUint32Max to macros.py; maybe it could be used here.
The CQ bit was checked by yangguo@chromium.org
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/1427573005/#ps200001 (title: "use kMaxUint32")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427573005/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427573005/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/9a569ec2c818e17fd2286459202b9fb5ba3d27af Cr-Commit-Position: refs/heads/master@{#31911} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
