On 2014/06/17 11:47:38, marja wrote:
Thanks for posting a new patch! Here are some comments again. Sorry,
they're a
bit high-level still... I'm not convinced of this ParamListFinder
approach.
Overall, the code is quite a bit cleaner now.
ParamListFinder... I reckon it may not be the nicest thing on Earth.
https://codereview.chromium.org/160073006/diff/500001/src/parser.cc#newcode3323
src/parser.cc:3323: error_token = "";
In this case, don't we get an incorrect error end position?
ParamListFinder in the current implementation will give this error instead
(that code path should not be reached):
(d8):1: SyntaxError: Function parameter list is malformed
(32, a) => {}
^^^^^^
SyntaxError: Function parameter list is malformed
Unfortunately, the error does not point at the exact point where the
issue is.
https://codereview.chromium.org/160073006/diff/500001/src/scanner.cc#newcode935
src/scanner.cc:935: bool Scanner::IdentifierIsFutureStrictReserved(const
AstString* string) const {
This sounds wrong, since we have a separate token for
FUTURE_RESERVED_WORD, so
here we have somehow lost that information and we're trying to get it
back..
:/
We are losing the information when lifting things up to the AST or
to the PreParserIdentifier/PreParserExpression objects. Would it be
acceptable to keep in the Scanner the positions of the last seen
future reserved word, last "eval" token, etc? That would be enough
(I think) for detecting those as part of a parameter list in strict
mode.
https://codereview.chromium.org/160073006/diff/500001/src/scanner.h#newcode694
src/scanner.h:694: void ParamListFinder::Update(Scanner* scanner) {
So you have this state machine which is updated for each token when
scanning,
and then there is ParserTraits::ParameterListFromExpression too, for sort
of
getting the same data "after the fact". This looks pretty duplicated to
me.
Also, you're adding this to be executed after each token during scanning,
and
I'm worried about the overhead.
Would it be feasible to
1) only do the parameter list dance if we have some indicator that we are
inside
the parameter list (seen "(", for example, and quit as soon as we realize
it's
not a valid parameter list)
2) do it either when reading the parameter list, or after, but not both?
And I'm not sure if Scanner is the right place for the parameter list
logic...
What's the reason why the "after the fact" processing is not sufficient?
Is it
because of PreParser? Did you try out the "PreParserExpression keeps
track of
the "this is a comma-separated identifier name list" property" approach,
and
why
didn't it fly?
The problem here is that PreParser (PreParserIdentifier/PreParserExpression)
does not tracks enough information: a single PreParserExpression is returned
from ParseAssignmentExpression(), while in Parser it returns an AST subtree
of binops/variableproxies. So I would need it to actually return a list of
identifiers seen (or something equivalent), their positions,
parenthesization,
and for them to have associated strings with the identifier names -- not just
an integer value with a set of bit flags.
That would make the code of PreParser::ParseAssignmentExpression() --and the
methods it uses-- more complex, and they would end handling a set of data
quite similar to what Parser does... and that made me wonder why to have
a PreParser to begin with, and to try to come up with the current solution.
Btw, how much does this regress the scanning performance? There's this
tool
lexer-shell which you can use for finding out..
As a quick test, I have run lexer-shell and parser-shell on lua.vm.js
(a version of the Lua VM compiled to JS using Emscripten, ~700kB in a
single JS file -- https://github.com/kripken/lua.vm.js). Those are the
results before and after my patch:
before after
lexer-shell, release 8ms 11ms
lexer-shell, debug 40ms 50ms
parser-shell, release 37/ 32ms 40/ 34ms
parser-shell, debug 267/248ms 281/262ms
https://codereview.chromium.org/160073006/diff/500001/test/cctest/test-parsing.cc#newcode257
test/cctest/test-parsing.cc:257: NULL
What, why? What are the changes in this test?
Ouch, this slipped in during a rebase.
https://codereview.chromium.org/160073006/diff/500001/test/cctest/test-parsing.cc#newcode1220
test/cctest/test-parsing.cc:1220:
parser->set_allow_arrow_functions(i::FLAG_harmony_arrow_functions);
We'd probably like to run each existing ParserSyncTest both with and
without
arrow functions, so you should handle it like the flags above (add a flag
for
it
into "flags" here).
But I realize it's actually not as easy as it should be, because for some
tests
we want to pin some flags (so that other flags vary but those are always
true).
That's why I decided to go for always setting the flag for arrow functions
to true.
I can try to add the machinery for that (an "alwaysTrueFlags" array which
overrides the varying flags) and see how it looks.
Sure, will do.
https://codereview.chromium.org/160073006/diff/500001/test/cctest/test-parsing.cc#newcode2063
test/cctest/test-parsing.cc:2063: {"eval => {", "}"},
You could have a separate test case for these, and remove the i::FLAG
above
(so
that the main test case would be both with and without the arrow function
flag).
Okay.
https://codereview.chromium.org/160073006/diff/500001/test/cctest/test-parsing.cc#newcode2637
test/cctest/test-parsing.cc:2637:
Do any of the cases bump into the "is not a variable proxy" in
ParserTraits::ParameterListFromExpression()? I guess not? That would
happen
when
we expect a parameter name, but get some other kind of expression which
is not
a
parameter name, such as "13" or "foo ? bar : baz" or "foo + bar". Could
you
add
test cases for those?
I have already added a bunch of this kind of test cases. So far the
errors are reported as expected for non-VariableProxies :)
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.
