LG modulo these comments. There's one bug (missing setting the flag), and
the
rest is variable naming and style nits.
In general, the code looks very nice like arv@ said above, and IMO the
latest
patch set is a clear improvement over the previous versions.
Thanks for working on this!
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode3914
src/parser.cc:3914: allow_harmony_object_literals());
Here you need to set the allow_templates flag to reusable_preparser as
well.
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5062
src/parser.cc:5062: const ZoneList<Expression*>* cookedStrings =
lit->cooked();
Style: cookedStrings -> cooked_strings (+ other variables elsewhere)
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5070
src/parser.cc:5070: if (!expressions->length()) {
Style nit: if (expressions->length() == 0)
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5088
src/parser.cc:5088: ZoneList<Expression*>* rawStrings = new (zone())
ZoneList<Expression*>(
raw_strings
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5093
src/parser.cc:5093: for (int i = 0; i < cookedStrings->length(); ++i) {
Gets confusing with i, c and j; could you rename i to string_index or
sth, then j could be raw_chars_from_index and c could be
raw_chars_to_index. Or something along those lines.
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5101
src/parser.cc:5101: // Normalize line endings
You could expand this comment to say what exactly is changed to what.
It's not trivial to see from the code below.... \r\n is changed to \n,
right?
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5117
src/parser.cc:5117:
I like this version of raw string building thing better than the
previous version, since this is localized: this quirk of templates
doesn't leak to all places where we handle literals.
https://codereview.chromium.org/663683006/diff/400001/src/parser.cc#newcode5133
src/parser.cc:5133: ZoneList<Expression*>* callArgs = new (zone())
ZoneList<Expression*>(
call_args
https://codereview.chromium.org/663683006/diff/400001/src/scanner.cc
File src/scanner.cc (right):
https://codereview.chromium.org/663683006/diff/400001/src/scanner.cc#newcode836
src/scanner.cc:836: // consisting of the CV 0x000A.
I know this is directly from the spec, but... wut? What's the difference
between "the CV 0x000A" and "the sequence consisting of the CV 0x000A"?
https://codereview.chromium.org/663683006/
--
--
v8-dev mailing list
[email protected]
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 an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
