Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2427253003:
Simplify and fix the rewriter (Closed)
|
Created:
4 years, 2 months ago by Toon Verwaest Modified:
4 years, 1 month ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionSimplify and fix the rewriter
Now we
- always set .result to undefined before a visited loop and switch since we can't know whether they will set a value,
- only visit finally if it can break/continue; and only store/restore .result in that case
BUG=
Committed: https://crrev.com/caba112d0396168349971ca8c90c525490eb5c9f
Cr-Commit-Position: refs/heads/master@{#40542}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fix and add tests #
Messages
Total messages: 24 (10 generated)
verwaest@chromium.org changed reviewers: + adamk@chromium.org
ptal
adamk@chromium.org changed reviewers: + neis@chromium.org
+neis
https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:287: for (int i = 0; i < clauses->length(); ++i) { I think changing the order here is wrong. Consider this example: a: while(true) { switch(0) { case 0: 42; case 1: break a; }; 33 } Its value should be 42, but is now undefined. https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:315: replacement_ = set_after ? node : AssignUndefinedBefore(node); Why did you change this to look at the old value of is_set_?
https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:315: replacement_ = set_after ? node : AssignUndefinedBefore(node); On 2016/10/21 12:15:25, neis wrote: > Why did you change this to look at the old value of is_set_? Because is set has no meaning. That's just whether the first clause produced a value. See the added test.
https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:287: for (int i = 0; i < clauses->length(); ++i) { On 2016/10/21 12:15:25, neis wrote: > I think changing the order here is wrong. Consider this example: > > a: while(true) { switch(0) { case 0: 42; case 1: break a; }; 33 } > > Its value should be 42, but is now undefined. Good point, I'll add the test.
https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc File src/parsing/rewriter.cc (right): https://codereview.chromium.org/2427253003/diff/1/src/parsing/rewriter.cc#new... src/parsing/rewriter.cc:315: replacement_ = set_after ? node : AssignUndefinedBefore(node); On 2016/10/21 14:13:08, Toon Verwaest wrote: > On 2016/10/21 12:15:25, neis wrote: > > Why did you change this to look at the old value of is_set_? > > Because is set has no meaning. That's just whether the first clause produced a > value. See the added test. If is_set_ is true, we shouldn't need to assign undefined. Which test do you mean? There's no new test involving a 'with' statement.
Fixed the bugs and added tests. Thanks!
The CQ bit was checked by verwaest@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
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 verwaest@chromium.org
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
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27176)
lgtm
The CQ bit was checked by verwaest@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Simplify and fix the rewriter Now we - always set .result to undefined before a visited loop and switch since we can't know whether they will set a value, - only visit finally if it can break/continue; and only store/restore .result in that case BUG= ========== to ========== Simplify and fix the rewriter Now we - always set .result to undefined before a visited loop and switch since we can't know whether they will set a value, - only visit finally if it can break/continue; and only store/restore .result in that case BUG= Committed: https://crrev.com/caba112d0396168349971ca8c90c525490eb5c9f Cr-Commit-Position: refs/heads/master@{#40542} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/caba112d0396168349971ca8c90c525490eb5c9f Cr-Commit-Position: refs/heads/master@{#40542} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
