On 2014/06/26 at 14:38:14, marja wrote:
Alright, the latest patch set is much better! Yay!
Reading this made my day yesterday :-)
Could you do a round of cleanup, that is, make sure you're not adding dead
code, duplicate code, unintentional changes etc.
Indeed!
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.
Sounds good to me. Also I will remove the logic to set the parenthesization
level to zero (I can't think of a case in which we would want to reset the
counter).
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#newcode561
src/parser.h:561: V8_INLINE Handle<String> EmptyIdentifierString();
Isn't this dead code?
Not really, this is used by the code generation in the follow-up CL that I
will
be posting after this is landed. Of course it does not really make sense to
introduce this while it is not used, so I am moving it to the follow-up CL.
https://codereview.chromium.org/160073006/diff/540001/src/parser.h#newcode573
src/parser.h:573: int pos = RelocInfo::kNoPosition);
Why is the position needed?
It is not needed in the latest version of the CL, I am removing this bit.
https://codereview.chromium.org/160073006/diff/540001/src/parser.h#newcode595
src/parser.h:595: Vector<VariableProxy*> ParameterListFromExpression(
Expression* expression);
Nit: extra space.
Oh, somehow the presubmit checks did not catch this.
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?
This is stray, yes. But InferFunctionName() is used in the code generation
patch, so I will be moving that to the follow-up CL as well.
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?
Sure, I can add a NULL default value for the constructor.
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...?)
This is to be used in the follow-up CL in Parser::ParseLazy(), like this:
if (shared_info->is_arrow())
result =
reinterpret_cast<FunctionLiteral*>(ParseArrowFunctionLiteral(&ok));
else
result = ParseFunctionLiteral( /* ... */ );
I have been thinking a bit about this, and IMHO it would be better to avoid
having two code paths. The solution would be to just call
ParseAssignmentExpression(), like this:
if (shared_info->is_arrow())
result =
reinterpret_cast<FunctionLiteral*>(ParseAssignmentExpression(false, &ok));
else
result = ParseFunctionLiteral( /* ... */ );
Anyway, this alternate version of ParseArrowFunctionLiteral() is dead code
for
the purpose of this CL, so I am removing it.
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?)
The way error reporting is done now does not need the lengths, so you are
right
and this may as well return 0.
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?
Right, it is only set when "(function ..." is found, so it will never be
true
for arrow functions. I think at some point I had in mind that it would be
nice
to detect cases like "(() => {})()" in which the arrow function is called
right
away, but it is certainly more difficult that for normal functions. Also, in
most cases we probably want arrow functions parsed lazily even if they are
parenthesized, like in the following example where two arrow functions are
parenthesized and it can happen that one of them is never used:
var a = [1, 2, 3, 4, 5].map(doSquares ? (item => item * item) : (item =>
item));
In conclusion: let's leave this out for now and not do premature
optimizations.
If a similar optimization is needed later on for arrow functions, add it
with a
benchmark that demonstrates the validity of the optimization.
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?
That was unintentional.
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).
As you can image, I understood it would be good to have arrow functions
enabled
in all tests. I'll remove the flag for the ones which do not involve arrow
functions.
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 :)
Reverting. In the follow-up CL with the codegen I think this can be added
back,
to test that the preparse data is used for arrow functions as well.
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.
Ok.
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.
Indeed.
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.
