One nit for a followup perhaps.
https://codereview.chromium.org/1189743003/diff/11/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1189743003/diff/11/src/preparser.h#newcode3552
src/preparser.h:3552: ReportUnexpectedToken(next);
Nit: will this give the
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1189743003/120001
https://codereview.chromium.org/1189743003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are
Comments addressed.
Initializers (+default parameters) have completely wrong scope so far. I'll
address everything relating to initializers in the follow-up
https://codereview.chromium.org/1189743003/diff/80001/src/parser.cc
File src/parser.cc (right):
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1189743003/11
https://codereview.chromium.org/1189743003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are
Try jobs failed on following builders:
v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/1566)
https://codereview.chromium.org/1189743003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
Sending half-done review, now to look at rebase. Overall looks pretty
swell but
one question below.
https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1189743003/diff/60001/src/preparser.h#newcode3522
https://codereview.chromium.org/1189743003/diff/80001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/1189743003/diff/80001/test/cctest/test-parsing.cc#newcode6608
test/cctest/test-parsing.cc:6608: TEST(DestructuringDuplicateParams) {
Need
LGTM with nit above about rest patterns.
That said :) Is this expected to work for initializers of eagerly-parsed
arrow
functions? I would expect that the literal_count() or the property_count()
of
the initializers would be recorded in the outer scope and not in the arrow
function's
Committed patchset #7 (id:120001)
https://codereview.chromium.org/1189743003/
--
--
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/42f30f4ded2b1ca0c4caa7639e6206e93c78ee70
Cr-Commit-Position: refs/heads/master@{#29184}
https://codereview.chromium.org/1189743003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this
Comments addressed (sorry had to rebase).
PTAL
https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc#newcode4301
src/parser.cc:4301:
On 2015/06/19 19:25:26, arv wrote:
-1 newline
A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1195163007/ by machenb...@chromium.org.
The reason for reverting is: [Sheriff] Breaks tsan:
http://build.chromium.org/p/client.v8/builders/V8%20Linux64%20TSAN/builds/4392.
PS#8 attempts to fix a read-bogus-memory serialization issue.
Yang, ptal at a change in serialize.cc
https://codereview.chromium.org/1189743003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1189743003/140001
https://codereview.chromium.org/1189743003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are
Patchset 8 (id:??) landed as
https://crrev.com/e7cdb615aeb18f7b4089eb646489d78a853050c9
Cr-Commit-Position: refs/heads/master@{#29192}
https://codereview.chromium.org/1189743003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this
Committed patchset #8 (id:140001)
https://codereview.chromium.org/1189743003/
--
--
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
Please take another look
https://codereview.chromium.org/1189743003/diff/1/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1189743003/diff/1/src/parser.cc#newcode4227
src/parser.cc:4227: descriptor.mode = VAR;
Changing to LET - it is always LET for patterns since it
Looks good, but I'm on my way out and only had time for a superficial look.
I
think Andy should have closer look as well.
https://codereview.chromium.org/1189743003/diff/60001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
LGTM
If you want to handle the property name issue later that is fine with me.
https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1189743003/diff/60001/src/parser.cc#newcode2057
src/parser.cc:2057: Expression*
Refactored formal parameter parsing to better encapsulate state and separate
concerns.
Will now address other comments.
https://codereview.chromium.org/1189743003/diff/1/src/scopes.h
File src/scopes.h (right):
https://codereview.chromium.org/1189743003/diff/1/src/scopes.h#newcode377
Now we can add tests for duplicate param names with destructuring.
// sloppy
function f(x, {x}) { // SyntaxError
}
https://codereview.chromium.org/1189743003/diff/1/test/mjsunit/harmony/destructuring.js
File test/mjsunit/harmony/destructuring.js (right):
https://codereview.chromium.org/1189743003/diff/1/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1189743003/diff/1/src/parser.cc#newcode4227
src/parser.cc:4227: descriptor.mode = VAR;
On 2015/06/16 17:02:12, rossberg wrote:
Shouldn't this be LET?
I think this is
https://codereview.chromium.org/1189743003/diff/1/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/1189743003/diff/1/src/parser.cc#newcode4227
src/parser.cc:4227: descriptor.mode = VAR;
Shouldn't this be LET?
23 matches
Mail list logo
