Alright, here are some more comments. These are pretty much trivial. The
biggest
question mark at this point is the raw literal value collection vs. reading
it
from the source code. Pls also see my other comment in the previous patch
set.
In general this looks pretty good, I'm generally convinced about this CL.
And
the test is good!
https://codereview.chromium.org/663683006/diff/260001/src/ast-value-factory.h
File src/ast-value-factory.h (right):
https://codereview.chromium.org/663683006/diff/260001/src/ast-value-factory.h#newcode251
src/ast-value-factory.h:251: F(GetTemplateCallSite,
"GetTemplateCallSite") \
You'd probably want to change the first part to get_template_call_site,
for consistency (see below make_reference_error etc.).
https://codereview.chromium.org/663683006/diff/260001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode1400
src/preparser.h:1400: void AddTemplateSpan(TemplateLiteralState* state)
{ USE(state); }
Shorter (here and below):
void AddTemplateSpan(TemplateLiteralState*) {}
(if you omit the parameter name, you don't need USE)
https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode2877
src/preparser.h:2877: // Add TV / TRV
This comment is confusing... which part is adding the TV / TRV?
https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode2887
src/preparser.h:2887: for (;;) {
Style nit: while(true) seems to be more common.
https://codereview.chromium.org/663683006/diff/260001/src/preparser.h#newcode2895
src/preparser.h:2895: // Parse an Expression
Nit: this comment is not needed; it's clear that it parses an
expression. The comments should explain the intention (like the bigger
comment in the beginning of ParseTemplateLiteral does) and the should
explain why the code is written exactly the way it is but only if it's
nontrivial ("a must be done before b, because...").
https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc
File src/scanner.cc (right):
https://codereview.chromium.org/663683006/diff/260001/src/scanner.cc#newcode819
src/scanner.cc:819: PushBack('}');
Hmm, when does this happen and why do we PushBack?
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.
