Even better! The comments I have are pretty minor (basically just 2
non-trivial
ones), so I hope we get this sorted out soon.
Re: discussion
I don't think we need to make arrow functions lazy. They're small, so
switching
to PreParser for the function body is not going to make a difference. Let's
keep
it simple and not do lazy arrow functions.
https://codereview.chromium.org/160073006/diff/580001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/160073006/diff/580001/src/parser.cc#newcode3322
src/parser.cc:3322: Vector<VariableProxy*>
ParserTraits::ParameterListFromExpression(
Comment from the previous round:
Here you have the same logic in 2 functions; would it be possible to
have only
one function which both checks if the expression is a parameter list and
constructs the vector?
https://codereview.chromium.org/160073006/diff/580001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/160073006/diff/580001/src/preparser.h#newcode893
src/preparser.h:893: // TODO(aperezdc): Would allowing lazy compilation
in preparser make sense?
Doesn't seem very sensemaking to me, when we are in PreParser, we're
already parsing a lazy function and cannot get lazier than that :)
https://codereview.chromium.org/160073006/diff/580001/src/preparser.h#newcode899
src/preparser.h:899: bool IsDeclared(const PreParserIdentifier&
identifier) const {
What cases does this IsDeclared actually detect? Pls add a comment.
https://codereview.chromium.org/160073006/diff/580001/src/preparser.h#newcode1274
src/preparser.h:1274: bool
IsValidArrowFunctionParameterList(PreParserExpression expression) {
Pls reorder these functions so that they're in the same order in
ParserTraits and PreParserTraits. (So this Is... function should go
where the other IsFoo functions are).
https://codereview.chromium.org/160073006/diff/580001/src/preparser.h#newcode1280
src/preparser.h:1280: void CheckConflictingVarDeclarations(
Nit: formatting.
You'd probably want to have a look how you can use clang-format in your
favourite editor :)
https://codereview.chromium.org/160073006/diff/580001/src/preparser.h#newcode2539
src/preparser.h:2539:
this->CheckStrictFunctionNameAndParameters(this->EmptyIdentifier(),
This is not inside if (strict_mode() == STRICT) because arrow function
parameter lists are always strict, right? Pls add a comment.
https://codereview.chromium.org/160073006/diff/580001/src/preparser.h#newcode2557
src/preparser.h:2557: return this->EmptyExpression();
This will change when you start generating the arrow function AST for
real, right? Pls add a comment.
https://codereview.chromium.org/160073006/diff/580001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/160073006/diff/580001/test/cctest/test-parsing.cc#newcode975
test/cctest/test-parsing.cc:975: { " start;\n"
Why this change?
https://codereview.chromium.org/160073006/diff/580001/test/cctest/test-parsing.cc#newcode1457
test/cctest/test-parsing.cc:1457:
This file seems to contain some unintentional changes still..
https://codereview.chromium.org/160073006/diff/580001/test/cctest/test-parsing.cc#newcode2817
test/cctest/test-parsing.cc:2817: TEST(ErrorsArrowFunctions) {
I like this test! :)
https://codereview.chromium.org/160073006/
--
--
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.
