Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1666183002:
[es6] Fix tail Call nodes marking. (Closed)
|
Created:
4 years, 10 months ago by Igor Sheludko Modified:
4 years, 10 months ago 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] Fix tail Call nodes marking.
TBR=rossberg@chromium.org
Committed: https://crrev.com/3c71bd184638deab45748e3a0776e4470ddef879
Cr-Commit-Position: refs/heads/master@{#33761}
Patch Set 1 : #
Messages
Total messages: 27 (13 generated)
The CQ bit was checked by ishell@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/1666183002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666183002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/2491)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by ishell@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/1666183002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666183002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ishell@chromium.org changed reviewers: + bmeurer@chromium.org
PTAL
Awesome. LGTM.
ishell@chromium.org changed reviewers: + littledan@chromium.org
Dan, PTAL at parser part.
Description was changed from ========== [es6] Fix tail Call nodes marking. ========== to ========== [es6] Fix tail Call nodes marking. TBD=rossberg@chromium.org ==========
Description was changed from ========== [es6] Fix tail Call nodes marking. TBD=rossberg@chromium.org ========== to ========== [es6] Fix tail Call nodes marking. TBR=rossberg@chromium.org ==========
The CQ bit was checked by ishell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666183002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666183002/20001
ishell@chromium.org changed reviewers: + rossberg@chromium.org
TBRed Andreas, landing
Message was sent while issue was closed.
Description was changed from ========== [es6] Fix tail Call nodes marking. TBR=rossberg@chromium.org ========== to ========== [es6] Fix tail Call nodes marking. TBR=rossberg@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [es6] Fix tail Call nodes marking. TBR=rossberg@chromium.org ========== to ========== [es6] Fix tail Call nodes marking. TBR=rossberg@chromium.org Committed: https://crrev.com/3c71bd184638deab45748e3a0776e4470ddef879 Cr-Commit-Position: refs/heads/master@{#33761} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3c71bd184638deab45748e3a0776e4470ddef879 Cr-Commit-Position: refs/heads/master@{#33761}
Message was sent while issue was closed.
I'm afraid this CL is based on a misunderstanding of the marking. The idea was:
- Call MarkTail on the 'return' argument.
- Have it propagate through all the syntactic constructs that can appear there.
In particular, _all_ propagation through _statements_ is only needed for
do-expressions. E.g., such that
return do { a; f() }
marks f as a tail call.
That is, I think this CL is not a fix at all, instead it completely breaks the
logic. Can you provide an example why you thought this change was necessary?
Message was sent while issue was closed.
On 2016/02/05 15:05:52, rossberg wrote: > Can you provide an example why you thought this change was necessary? Okay, I see now the problem you tried to fix, namely that Returns were marked unconditionally, even if they occurred in a try-block. I think the proper way to fix this is introducing a bit in the function state that keeps track of being inside a try-block, and only mark returns in the correct case.
Message was sent while issue was closed.
On 2016/02/05 15:21:25, rossberg wrote: > On 2016/02/05 15:05:52, rossberg wrote: > > Can you provide an example why you thought this change was necessary? > > Okay, I see now the problem you tried to fix, namely that Returns were marked > unconditionally, even if they occurred in a try-block. > > I think the proper way to fix this is introducing a bit in the function state > that keeps track of being inside a try-block, and only mark returns in the > correct case. Indeed, I broke do expressions. I'll fix this on Monday. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
