Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2616673003:
[promises] Move various promise reject functions to TF (Closed)
|
Created:
3 years, 11 months ago by gsathya Modified:
3 years, 11 months ago Reviewers:
Igor Sheludko CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[promises] Move various promise reject functions to TF
BUG=v8:5343
Review-Url: https://codereview.chromium.org/2616673003
Cr-Commit-Position: refs/heads/master@{#42113}
Committed: https://chromium.googlesource.com/v8/v8/+/a5f3c4d10c4d710a602c910083409ee6c2e84b38
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : fix #Patch Set 4 : more fixes #
Total comments: 10
Patch Set 5 : fix nits #Patch Set 6 : rebase #Patch Set 7 : fix build #
Messages
Total messages: 35 (30 generated)
The CQ bit was checked by gsathya@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/18625) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
The CQ bit was checked by gsathya@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)
The CQ bit was checked by gsathya@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...
Description was changed from ========== [promises] Move various promise reject functions to TF BUG=v8:5343 ========== to ========== [promises] Move various promise reject functions to TF BUG=v8:5343 ==========
gsathya@chromium.org changed reviewers: + ishell@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits: https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:867: LoadFixedArrayElement(context, has_already_visited_slot); LoadContextElement https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:870: GotoIf(SmiEqual(has_already_visited, SmiConstant(1)), &out); Nit: this should be GotoUnless(SmiEqual(..., SmiConstant(0)) to match the PromiseUtils::HasAlreadyVisited() implementation. https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:873: StoreFixedArrayElement(context, has_already_visited_slot, SmiConstant(1)); StoreContextElementNoWriteBarrier https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:876: Node* const promise = LoadFixedArrayElement( LoadContextElement https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:878: Node* const debug_event = LoadFixedArrayElement( Same here.
The CQ bit was checked by gsathya@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...
updated PromiseResolveClosure as well https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (right): https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:867: LoadFixedArrayElement(context, has_already_visited_slot); On 2017/01/06 17:26:25, Igor Sheludko wrote: > LoadContextElement Done. https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:870: GotoIf(SmiEqual(has_already_visited, SmiConstant(1)), &out); On 2017/01/06 17:26:25, Igor Sheludko wrote: > Nit: this should be > GotoUnless(SmiEqual(..., SmiConstant(0)) > to match the PromiseUtils::HasAlreadyVisited() implementation. Leaving as such to match PromiseResolveClosure implementation. https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:873: StoreFixedArrayElement(context, has_already_visited_slot, SmiConstant(1)); On 2017/01/06 17:26:25, Igor Sheludko wrote: > StoreContextElementNoWriteBarrier Done. https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:876: Node* const promise = LoadFixedArrayElement( On 2017/01/06 17:26:25, Igor Sheludko wrote: > LoadContextElement Done. https://codereview.chromium.org/2616673003/diff/60001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:878: Node* const debug_event = LoadFixedArrayElement( On 2017/01/06 17:26:25, Igor Sheludko wrote: > Same here. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
The CQ bit was checked by gsathya@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
The CQ bit was checked by gsathya@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...
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 gsathya@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org Link to the patchset: https://codereview.chromium.org/2616673003/#ps120001 (title: "fix build")
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": 120001, "attempt_start_ts": 1483733084633430,
"parent_rev": "45c11887929c1f0fbc54849d2627cd8bb90bcc37", "commit_rev":
"a5f3c4d10c4d710a602c910083409ee6c2e84b38"}
Message was sent while issue was closed.
Description was changed from ========== [promises] Move various promise reject functions to TF BUG=v8:5343 ========== to ========== [promises] Move various promise reject functions to TF BUG=v8:5343 Review-Url: https://codereview.chromium.org/2616673003 Cr-Commit-Position: refs/heads/master@{#42113} Committed: https://chromium.googlesource.com/v8/v8/+/a5f3c4d10c4d710a602c910083409ee6c2e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/v8/v8/+/a5f3c4d10c4d710a602c910083409ee6c2e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
