Thanks --- I'll spend some time addressing these comments, but I'm still
leaning
more towards the alt. implementation
https://codereview.chromium.org/1309813007/diff/150001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/1309813007/diff/150001/src/ast.h#newcode227
src/ast.h:227: inline bool IsPatternOrLiteral() const {
On 2015/10/27 23:04:18, adamk wrote:
Nit: "inline" is redundant here
Acknowledged.
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
File src/preparser.h (left):
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#oldcode2684
src/preparser.h:2684: this->ExpressionUnexpectedToken(classifier);
On 2015/10/27 23:04:18, adamk wrote:
Sorry, what happened here? Is this supposed to be dependent on the
allow_destructuring_assignment flag?
We can't record this as an ExpressionError here, because it breaks
CoverInitializedNames in arrow formals.
The workaround is the ArrowFormalParametersState thing (in this patch
set), or the `kMaybeBindingPattern` flag in the 2nd version, which I
like better. It's just used to defer reporting of the error.
I haven't tested if it still reports the `=` as an error if the flag
isn't used, but I expect it does.
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcode777
src/preparser.h:777: #define DECLARE_BOOLEAN_STATE_ON_STACK(type,
state_var) \
On 2015/10/27 23:04:18, adamk wrote:
This whole thing looks pretty big for what it's doing.
I'm not convinced you need to add Push and Pop, for one, since it
seems like you
should always be able to use RAII. And the Is() accessor seems like
unnecessary
boilerplate given that the state is directly available from the
parser.
Overall I think this stuff should be as minimal as possible (if you
could get
away without the macro that'd be even better).
Yeah, true
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcode800
src/preparser.h:800: friend class type##State
On 2015/10/27 23:04:18, adamk wrote:
No need to friend an inner class.
Really? I could have sworn I've seen errors from this kind of thing
before
https://codereview.chromium.org/1309813007/diff/150001/src/preparser.h#newcode2477
src/preparser.h:2477: bool old_state_ = PushArrowFormalsState(true);
On 2015/10/27 23:04:18, adamk wrote:
Can't you wrap the next few lines in a block and use an RAII object
here instead
of calling these directly?
As mentioned, the second version gets rid of all this extra parser
state, it may be a simpler. Which one do you think would be preferred?
https://codereview.chromium.org/1309813007/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
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 v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
