Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1738463003:
Ship ES2015 iterator finalization (Closed)
|
Created:
4 years, 10 months ago by Dan Ehrenberg Modified:
4 years, 9 months ago CC:
v8-reviews_googlegroups.com, rmcilroy Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionShip ES2015 iterator finalization
This patch moves iterator finalization (calling .return() when a
for-of loop exits early) to shipping. The only part of this feature
which is currently known to be missing is destructuring--.return()
should be also be called when destructuring with an array which
does not end in a rest pattern, but it currently does not. The rest
of this feature, including calling .return() from certain builtins,
is implemented.
R=adamk
BUG=v8:3566
LOG=Y
Committed: https://crrev.com/227fd1d4ed369ac9a6fe9ad432d1dc960cad385e
Cr-Commit-Position: refs/heads/master@{#34307}
Patch Set 1 #Patch Set 2 : Fix test expectations #Patch Set 3 : Fix cctest.status #
Total comments: 1
Messages
Total messages: 32 (13 generated)
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/patch-status/1738463003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738463003/1
Description was changed from ========== Ship ES2015 iterator finalization This patch moves iterator finalization (calling .return() when a for-of loop exits early) to shipping. The only part of this feature which is currently known to be missing is destructuring--.return() should be also be called when destructuring with an array which does not end in a rest pattern, but it currently does not. R=adamk BUG=v8:3566 LOG=Y ========== to ========== Ship ES2015 iterator finalization This patch moves iterator finalization (calling .return() when a for-of loop exits early) to shipping. The only part of this feature which is currently known to be missing is destructuring--.return() should be also be called when destructuring with an array which does not end in a rest pattern, but it currently does not. The rest of this feature, including calling .return() from certain builtins, is implemented. R=adamk BUG=v8:3566 LOG=Y ==========
lgtm
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1738463003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738463003/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
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/patch-status/1738463003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738463003/20001
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/11525)
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/patch-status/1738463003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738463003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1738463003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1738463003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Ship ES2015 iterator finalization This patch moves iterator finalization (calling .return() when a for-of loop exits early) to shipping. The only part of this feature which is currently known to be missing is destructuring--.return() should be also be called when destructuring with an array which does not end in a rest pattern, but it currently does not. The rest of this feature, including calling .return() from certain builtins, is implemented. R=adamk BUG=v8:3566 LOG=Y ========== to ========== Ship ES2015 iterator finalization This patch moves iterator finalization (calling .return() when a for-of loop exits early) to shipping. The only part of this feature which is currently known to be missing is destructuring--.return() should be also be called when destructuring with an array which does not end in a rest pattern, but it currently does not. The rest of this feature, including calling .return() from certain builtins, is implemented. R=adamk BUG=v8:3566 LOG=Y Committed: https://crrev.com/227fd1d4ed369ac9a6fe9ad432d1dc960cad385e Cr-Commit-Position: refs/heads/master@{#34307} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/227fd1d4ed369ac9a6fe9ad432d1dc960cad385e Cr-Commit-Position: refs/heads/master@{#34307}
Message was sent while issue was closed.
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1738463003/diff/40001/test/cctest/cctest.status File test/cctest/cctest.status (right): https://codereview.chromium.org/1738463003/diff/40001/test/cctest/cctest.stat... test/cctest/cctest.status:98: 'test-bytecode-generator/ForOf': [FAIL], Doing this shouldn't be necessary any longer (please see Stefano's mail to v8-dev about generate-bytecode-expectations --rebaseline). I've uploaded https://codereview.chromium.org/1738143002 to make this change.
Message was sent while issue was closed.
jkummerow@chromium.org changed reviewers: + jkummerow@chromium.org
Message was sent while issue was closed.
DBC: as a general rule, CLs titled "Ship $whatever" should not land on the day before a branch point (which falls into the V8-side blackout period of no Canary coverage).
Message was sent while issue was closed.
On 2016/03/01 at 10:08:04, jkummerow wrote: > DBC: as a general rule, CLs titled "Ship $whatever" should not land on the day before a branch point (which falls into the V8-side blackout period of no Canary coverage). This was done under the theory that we'd get one day of Canary coverage. But maybe it was ill-advised, and we should do it further ahead. What should be the cutoff date exactly? The official feature freeze point? In the end, we took an earlier Canary, and the feature is not shipping in 51.
Message was sent while issue was closed.
> This was done under the theory that we'd get one day of Canary coverage. But > maybe it was ill-advised, and we should do it further ahead. What should be the > cutoff date exactly? The official feature freeze point? In the end, we took an > earlier Canary, and the feature is not shipping in 51. Yeah, "stage on Wednesday, ship on Thursday, branch on Friday" is way too aggressive. Also, as you saw, it doesn't pan out -- what you land on Thursday won't even make it into the Friday Canary, and we often end up having to pick an earlier Canary as the basis for our branch anyway. There's no hard deadline in V8 for when things have to land, as we do strive to have branches have as little of an impact as possible on our workflow, but that said, "the week before the branch" is a good rule of thumb, more than that for scary features. We've had enough branch week firedrills in the past, it's really good to avoid those.
Message was sent while issue was closed.
On 2016/03/01 at 16:45:36, jkummerow wrote: > > This was done under the theory that we'd get one day of Canary coverage. But > > maybe it was ill-advised, and we should do it further ahead. What should be the > > cutoff date exactly? The official feature freeze point? In the end, we took an > > earlier Canary, and the feature is not shipping in 51. > > Yeah, "stage on Wednesday, ship on Thursday, branch on Friday" is way too aggressive. Also, as you saw, it doesn't pan out -- what you land on Thursday won't even make it into the Friday Canary, and we often end up having to pick an earlier Canary as the basis for our branch anyway. > > There's no hard deadline in V8 for when things have to land, as we do strive to have branches have as little of an impact as possible on our workflow, but that said, "the week before the branch" is a good rule of thumb, more than that for scary features. We've had enough branch week firedrills in the past, it's really good to avoid those. I definitely don't want to cause any fire drills. I thought this would be an easy revert. I don't think we have a consistent policy about when the cutoff is for language features; for example, I think Proxies were shipped rather last-minute too. In this case, I don't think any issues were caused. I'd be fine with a policy that, 'the latest date you can ship a feature is a week before branch'. What do others think?
Message was sent while issue was closed.
On 2016/03/01 16:50:14, Dan Ehrenberg wrote: > On 2016/03/01 at 16:45:36, jkummerow wrote: > > > This was done under the theory that we'd get one day of Canary coverage. But > > > maybe it was ill-advised, and we should do it further ahead. What should be > the > > > cutoff date exactly? The official feature freeze point? In the end, we took > an > > > earlier Canary, and the feature is not shipping in 51. > > > > Yeah, "stage on Wednesday, ship on Thursday, branch on Friday" is way too > aggressive. Also, as you saw, it doesn't pan out -- what you land on Thursday > won't even make it into the Friday Canary, and we often end up having to pick an > earlier Canary as the basis for our branch anyway. > > > > There's no hard deadline in V8 for when things have to land, as we do strive > to have branches have as little of an impact as possible on our workflow, but > that said, "the week before the branch" is a good rule of thumb, more than that > for scary features. We've had enough branch week firedrills in the past, it's > really good to avoid those. > > I definitely don't want to cause any fire drills. I thought this would be an > easy revert. I don't think we have a consistent policy about when the cutoff is > for language features; for example, I think Proxies were shipped rather > last-minute too. In this case, I don't think any issues were caused. > > I'd be fine with a policy that, 'the latest date you can ship a feature is a > week before branch'. What do others think? We had a discussion about this ... two branches before. Essentially we are selecting a backup candidate on Tuesday in the branch week. If there is not backup candidate available (because Stability) we enter a 'no risky changes mode'. If there is a backup candidate, it is business as usual. Just don't depend on the fact that your CL will in it if it lands after the backup candidate. Generally staging it for only one day and shipping it the next is not a good idea. Let's continue this discussion internally. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
