Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640013003:
[wasm] Some simplifications in function-body-decoder.cc. (Closed)
Description[wasm] Some simplifications in function-body-decoder.cc.
R=rossberg@chromium.org,clemensh@chromium.org
BUG=
Review-Url: https://codereview.chromium.org/2640013003
Cr-Commit-Position: refs/heads/master@{#42473}
Committed: https://chromium.googlesource.com/v8/v8/+/aa3cd2cd0745266fe98a5fc1304a889de431bcea
Patch Set 1 #
Total comments: 6
Patch Set 2 : Some more simplifications #Patch Set 3 : Regression test had unreachable code. #
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by titzer@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_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...) v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... File src/wasm/function-body-decoder.cc (right): https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... src/wasm/function-body-decoder.cc:659: while (true) { // decoding loop. You can also simplify this loop by removing the if-statement above, and making this "while (pc_ < end_)". See below for more. https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... src/wasm/function-body-decoder.cc:1242: if (pc_ >= end_) { This condition could go away then, just put this after the loop: if (pc_ > end_) error("Beyond end of code");
lgtm https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... File src/wasm/function-body-decoder.cc (right): https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... src/wasm/function-body-decoder.cc:849: // If the last (implicit) control was popped, check we are at end. Nit: s/was/is/
https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... File src/wasm/function-body-decoder.cc (right): https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... src/wasm/function-body-decoder.cc:659: while (true) { // decoding loop. On 2017/01/18 15:46:26, Clemens Hammacher wrote: > You can also simplify this loop by removing the if-statement above, and making > this "while (pc_ < end_)". See below for more. Yeah, I originally did this on purpose as a manual loop rotation, but it's better to just let the C++ compiler do it. https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... src/wasm/function-body-decoder.cc:849: // If the last (implicit) control was popped, check we are at end. On 2017/01/18 16:21:42, rossberg wrote: > Nit: s/was/is/ Done. https://codereview.chromium.org/2640013003/diff/1/src/wasm/function-body-deco... src/wasm/function-body-decoder.cc:1242: if (pc_ >= end_) { On 2017/01/18 15:46:26, Clemens Hammacher wrote: > This condition could go away then, just put this after the loop: > if (pc_ > end_) error("Beyond end of code"); Done.
The CQ bit was checked by titzer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/2640013003/#ps20001 (title: "Some more simplifications")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
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 titzer@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 titzer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clemensh@chromium.org, rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/2640013003/#ps40001 (title: "Regression test had unreachable code.")
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": 40001, "attempt_start_ts": 1484765625526380,
"parent_rev": "538f848ddf287c1ae7c1efb1a838dba46bf66bcb", "commit_rev":
"aa3cd2cd0745266fe98a5fc1304a889de431bcea"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Some simplifications in function-body-decoder.cc. R=rossberg@chromium.org,clemensh@chromium.org BUG= ========== to ========== [wasm] Some simplifications in function-body-decoder.cc. R=rossberg@chromium.org,clemensh@chromium.org BUG= Review-Url: https://codereview.chromium.org/2640013003 Cr-Commit-Position: refs/heads/master@{#42473} Committed: https://chromium.googlesource.com/v8/v8/+/aa3cd2cd0745266fe98a5fc1304a889de43... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/aa3cd2cd0745266fe98a5fc1304a889de43... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
