On 2014/07/04 at 08:21:14, marja wrote:
LGTM assuming you fix these comments.
Yay!
Thanks for working on this! It got a lot cleaner and more elegant in the
last
couple of patch sets.
Indeed. I agree myself on that. Thanks also to you for all the comments
while going back-and-forth until reaching the current state.
https://codereview.chromium.org/160073006/diff/600001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/160073006/diff/600001/src/preparser.h#newcode722
src/preparser.h:722: ? kMultiParenthesizedExpression
This is not very readable. Pls clang-format.
Afaics, it suggests:
code |= is_parenthesized() ? kMultiParenthesizedExpression
: kParenthesizedExpression;
If needed, you can add parens like this:
I will update with the clang-format suggestion, I think the extra
parenthesis do not contribute extra readability.
https://codereview.chromium.org/160073006/diff/600001/src/preparser.h#newcode2487
src/preparser.h:2487: // The vector has the items in reverse order.
Is it necessary to have the items in reverse order? It seems you always
add
the right side to the collector first, but couldn't you as easily add the
left
side first?
The AST subtree for comma-separated expressions has all leaves on the right
side, for example the expression:
(a, b, c, d)
would be parsed into:
,
/ \
, d
/ \
, c
/ \
a b
By handling the right leaves first, the depth of the call stack due to the
recursive invocation will never be more than one level for binop->right(),
and for the last recursive invocation on binop->left() the compiler can make
a tail-call optimization.
(Actually, at some point I had the algorithm written with a "while" loop,
but then I noticed that the compiler would optimize the recursive version
as well, and personally I find the recursive version easier to understand).
https://codereview.chromium.org/160073006/diff/600001/src/preparser.h#newcode2506
src/preparser.h:2506: CHECK_OK);
I don't think it makes sense to put CHECK_OK inside a return statement!
(See how the macro is defined.)
Indeed. I think it is better to follow the same style as in the rest of
the parser code, for coherence. Like this:
ExpressionT expression =
ParseArrowFunctionLiteralBody(... , CHECK_OK);
return expression;
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.
