CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1312613003/120001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1312613003/120001
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
Committed patchset #7 (id:120001)
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and
Patchset 7 (id:??) landed as
https://crrev.com/d6fb6de7097da908fd0a66804027ea189b559c0f
Cr-Commit-Position: refs/heads/master@{#30453}
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this
Your reasoning re: ScopeInfo sounds right to me. Can you add a note to
scopes.h
explaining why the bit isn't needed on ScopeInfo?
Also, could you add a few more tests (the ones suggested by Andreas)? In
particular, tests for assignment and loading inside evals (both sloppy and
strict would be
Just pinging on one of my comments so that it doesn't drown in the noise.
Still
LGTM after that of course.
https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-codegen.cc
File src/full-codegen/full-codegen.cc (right):
lgtm once you've added sloppy eval tests
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this
Final list of things that need to be fixed before landing:
- Renaming of NeedsHoleCheck method
- Test for sloppy eval
Also on the menu, but can be handled in a follow-up (since it's not related
to
hole-checking): add tests for completion value of switch, and fix the
desugaring
to handle
On 2015/08/27 at 16:21:04, adamk wrote:
Your reasoning re: ScopeInfo sounds right to me. Can you add a note to
scopes.h explaining why the bit isn't needed on ScopeInfo?
Also, could you add a few more tests (the ones suggested by Andreas)? In
particular, tests for assignment and loading
All required reviewers (with asterisk prefixes) have not yet approved this
CL.
No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1312613003/120001
View timeline at
https://chromium-cq-status.appspot.com/patch-timeline/1312613003/120001
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
lgtm, I think you've satisfied rossberg's requirements with the added tests
and
comment in scopes.h
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to
LGTM on full codegen, just nits. But one concern about scope serialization,
adding Andreas for that.
https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-codegen.cc
File src/full-codegen/full-codegen.cc (right):
https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
On 2015/08/26 11:30:30, Michael Starzinger wrote:
Isn't
On 2015/08/26 12:07:58, rossberg wrote:
https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/1312613003/diff/60001/src/scopes.h#newcode232
src/scopes.h:232: void SetNonlinear() { scope_nonlinear_ = true; }
On 2015/08/26
https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-codegen.h
File src/full-codegen/full-codegen.h (right):
https://codereview.chromium.org/1312613003/diff/60001/src/full-codegen/full-codegen.h#newcode704
src/full-codegen/full-codegen.h:704: bool
On 2015/08/27 at 01:40:49, Dan Ehrenberg wrote:
Could we leave tracking nonlinear scopes in ScopeInfo for a future patch?
This
one brings the world further towards correctness as-is. And I'm not sure
how to
trigger a test case--everything I try just works.
Could we leave tracking nonlinear scopes in ScopeInfo for a future patch?
This
one brings the world further towards correctness as-is. And I'm not sure
how to
trigger a test case--everything I try just works.
lgtm % nits
https://codereview.chromium.org/1312613003/diff/40001/src/full-codegen/full-codegen.cc
File src/full-codegen/full-codegen.cc (right):
https://codereview.chromium.org/1312613003/diff/40001/src/full-codegen/full-codegen.cc#newcode1601
src/full-codegen/full-codegen.cc:1601:
https://codereview.chromium.org/1312613003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups v8-dev group.
To unsubscribe from this group and stop receiving emails from it, send
https://codereview.chromium.org/1312613003/diff/40001/src/full-codegen/full-codegen.cc
File src/full-codegen/full-codegen.cc (right):
https://codereview.chromium.org/1312613003/diff/40001/src/full-codegen/full-codegen.cc#newcode1601
src/full-codegen/full-codegen.cc:1601: DCHECK(var-location()
Reviewers: adamk,
Message:
The test still fails on TurboFan; I'm investigating.
Description:
Ensure hole checks take place in switch statement scopes
Switch statements introduce their own scope for cases, but this scope
is not necessarily executed in order, as the following function shows:
https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/full-codegen-arm.cc
File src/full-codegen/arm/full-codegen-arm.cc (right):
https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/full-codegen-arm.cc#newcode1471
https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/full-codegen-arm.cc
File src/full-codegen/arm/full-codegen-arm.cc (right):
https://codereview.chromium.org/1312613003/diff/20001/src/full-codegen/arm/full-codegen-arm.cc#newcode1471
23 matches
Mail list logo
