Alright, the latest patch set is much better! Yay!
Could you do a round of cleanup, that is, make sure you're not adding dead
code,
duplicate code, unintentional changes etc.
Some comments below. I have a list of stuff for myself which I need to
check why
they are needed, but meanwhile I wanted to unblock you...
https://codereview.chromium.org/160073006/diff/540001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/160073006/diff/540001/src/ast.h#newcode350
src/ast.h:350: void set_is_parenthesized(bool value) {
Afaics this is never called with false (which would be a weird thing
anyway... like, we have increased the parenthization level before and
then we suddenly decide it's not parenthesized after all).
So you could call this increase_parenthesization_level() instead.
https://codereview.chromium.org/160073006/diff/540001/src/messages.js
File src/messages.js (right):
https://codereview.chromium.org/160073006/diff/540001/src/messages.js#newcode159
src/messages.js:159: strict_parameter_list: ["Malformed strict
mode parameter list"],
This error message is not about strict mode parameter lists, so you
should call it "malformed_arrow_function_parameter_list" and the message
should be "Malformed arrow function parameter list".
https://codereview.chromium.org/160073006/diff/540001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/160073006/diff/540001/src/parser.cc#newcode3310
src/parser.cc:3310: Vector<VariableProxy*>
ParserTraits::ParameterListFromExpression(
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/540001/src/parser.h
File src/parser.h (right):
https://codereview.chromium.org/160073006/diff/540001/src/parser.h#newcode561
src/parser.h:561: V8_INLINE Handle<String> EmptyIdentifierString();
Isn't this dead code?
https://codereview.chromium.org/160073006/diff/540001/src/parser.h#newcode573
src/parser.h:573: int pos = RelocInfo::kNoPosition);
Why is the position needed?
https://codereview.chromium.org/160073006/diff/540001/src/parser.h#newcode595
src/parser.h:595: Vector<VariableProxy*> ParameterListFromExpression(
Expression* expression);
Nit: extra space.
https://codereview.chromium.org/160073006/diff/540001/src/parser.h#newcode596
src/parser.h:596: bool IsValidArrowFunctionParameterList(Expression*
expression);
This function should go where the other IsThisAndThat functions are.
https://codereview.chromium.org/160073006/diff/540001/src/parser.h#newcode597
src/parser.h:597: void InferFunctionName(FunctionLiteral*
func_to_infer);
Isn't this a stray definition?
https://codereview.chromium.org/160073006/diff/540001/src/preparser.cc
File src/preparser.cc (right):
https://codereview.chromium.org/160073006/diff/540001/src/preparser.cc#newcode112
src/preparser.cc:112: NULL, this->ast_value_factory());
this->ast_value_factory is always NULL, right? So why not use a default
value for it in the FunctionState ctor?
https://codereview.chromium.org/160073006/diff/540001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/160073006/diff/540001/src/preparser.h#newcode466
src/preparser.h:466: ExpressionT ParseArrowFunctionLiteral(bool* ok);
Isn't this one dead code? (And isn't the comment above wrong...?)
https://codereview.chromium.org/160073006/diff/540001/src/preparser.h#newcode610
src/preparser.h:610: int length() const {
This is used for duplicate parameter error message positions, right? Is
it use elsewhere? Might be okay to just always return 0... the PreParser
doesn't check duplicate positions yet anyway. (Or does something
observable change if you return 0 here?)
https://codereview.chromium.org/160073006/diff/540001/src/preparser.h#newcode2476
src/preparser.h:2476: ParserBase<Traits>::ParseArrowFunctionLiteralBody(
Nit: reorder the functions here to match the order of the definitions.
https://codereview.chromium.org/160073006/diff/540001/src/preparser.h#newcode2500
src/preparser.h:2500: parenthesized_function_ = false; // This Was set
for this funciton only.
1) Why do you need to set parenthesized_function_ here? It's not set
outside arrow functions anyway, because it's only set when we see the
function keyword, right?
Afaics, where this code originally was, it used to detect these cases:
(function foo() { << this is parenthised, so parenthesized_function_ is
set to true
<< here, parenthesized_function_ is set to false
function bar() {}<< so that this function doesn't appear as
parenthesized
})();
But I don't think we need it here...
2) The same code is in the if branch and else branch
3) the comment is pretty malformed :)
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (left):
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc#oldcode916
test/cctest/test-parsing.cc:916: v8::internal::FLAG_harmony_scoping =
true;
Why this change?
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc#newcode199
test/cctest/test-parsing.cc:199: i::FLAG_harmony_arrow_functions = true;
I don't think you need to add this to existing tests.
When I said that you should make sure that existing tests run with arrow
functions enabled, I meant the RunParserSync tests.
For them, it should be enough to add this flag to the ParserFlagEnum and
default_flags (like you did).
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc#newcode257
test/cctest/test-parsing.cc:257: const char* good_code[] = {
I whined about this change before, I'm still whining about it :)
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc#newcode357
test/cctest/test-parsing.cc:357:
preparser.set_allow_arrow_functions(true);
And these shouldn't be needed either. For these tests, we don't run them
with all experimental features anyway, so no need to make an exception
for arrow functions.
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc#newcode1426
test/cctest/test-parsing.cc:1426: static const ParserFlag always_flags[]
= { kAllowArrowFunctions };
Instead of this, you should add the flag to flags1 below, so the test
gets run with and without it.
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc#newcode1700
test/cctest/test-parsing.cc:1700: i::FLAG_harmony_arrow_functions =
true;
Instead of this, you should use always_true_flags...
Otherwise this test takes longer than needed, because it's run with both
the flag and without the flag (via the default_flags permutation thingy)
and then this FLAG just overrides them.
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc#newcode1765
test/cctest/test-parsing.cc:1765: i::FLAG_harmony_arrow_functions =
true;
Here too... in all places, where you need the flag, you should use
always_true_flags.
https://codereview.chromium.org/160073006/diff/540001/test/cctest/test-parsing.cc#newcode1824
test/cctest/test-parsing.cc:1824: i::FLAG_harmony_arrow_functions =
true;
And this is completely unnecessary; the test should be ran both with and
without arrow functions (and the default_flags permutation takes care of
it if you remove this line).
(And this goes on throughout this file; pls see all occurrences of
i::FLAG_harmony_arrow_functions.)
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.
