I like this version much better. Moving forward, since you can easily
construct
invalid kind combinations, let's add a couple more assertions, though (see
below).
https://codereview.chromium.org/477263002/diff/160001/src/ast.h
File src/ast.h (right):
https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode184
src/ast.h:184: // For generating IDs for AstNodes.
Please try to avoid rebasing while review is in progress.
https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2464
src/ast.h:2464: DCHECK(!(is_arrow() && is_concise_method()));
Perhaps factor these conditions into a function IsWellformedFunctionKind
or IsValidFunctionKind in globals.h, so that it can be reused in a
couple of other places.
https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2493
src/ast.h:2493: class IsArrow : public BitField<bool, 6, 1> {};
Please don't define overlapping bitfields. In particular, these ones
silently break when somebody changes the values in the definition of
FunctionKind.
https://codereview.chromium.org/477263002/diff/160001/src/globals.h
File src/globals.h (right):
https://codereview.chromium.org/477263002/diff/160001/src/globals.h#newcode768
src/globals.h:768: inline bool IsArrowFunction(FunctionKind kind) {
Each of these functions should assert that kind is well-formed.
https://codereview.chromium.org/477263002/diff/160001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/477263002/diff/160001/src/objects-inl.h#newcode5598
src/objects-inl.h:5598: void SharedFunctionInfo::set_kind(FunctionKind
kind) {
Assert that kind is well-formed.
https://codereview.chromium.org/477263002/diff/160001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode1873
src/preparser.h:1873: typename ParserBase<Traits>::IdentifierT
ParserBase<Traits>::ParsePropertyName(
Nit: Given that it's called only in one place, I don't think this
wrapper is necessary.
https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode1897
src/preparser.h:1897: checker->CheckProperty(next, kValueProperty,
Can this check be factored out and moved after the if-chain, to avoid
its duplication?
https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode1903
src/preparser.h:1903: // Concise Method
Nit: V8 typically uses a style where a comment like this is placed
inside the respective branch.
https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode1937
src/preparser.h:1937: LiteralT key = this->EmptyLiteral();
Nit: use ?: instead of `if` to avoid the redundant init.
https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsing.cc
File test/cctest/test-parsing.cc (right):
https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsing.cc#newcode3397
test/cctest/test-parsing.cc:3397: const char* object_literal_data[] = {
Hm, well, it's not really object literals in there...
https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsing.cc#newcode3414
test/cctest/test-parsing.cc:3414: {"'use strict'; ({", "(x, y) {}});"},
Good idea, you should probably add that to your other tests as well,
even though it _should_ not matter there.
https://codereview.chromium.org/477263002/
--
--
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.
