I'm back to normal schedule now.
I believe I fixed all the issues you pointed out.
PTAL
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#newcode2464
src/ast.h:2464: DCHECK(!(is_arrow() && is_concise_method()));
On 2014/08/25 09:39:29, rossberg wrote:
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.
Done.
https://codereview.chromium.org/477263002/diff/160001/src/ast.h#newcode2493
src/ast.h:2493: class IsArrow : public BitField<bool, 6, 1> {};
On 2014/09/08 14:08:59, rossberg wrote:
On 2014/09/02 14:42:48, arv wrote:
> On 2014/08/25 09:39:29, rossberg wrote:
> > Please don't define overlapping bitfields. In particular, these
ones
silently
> > break when somebody changes the values in the definition of
FunctionKind.
>
> Yes, it does have a tight coupling but it leads to more concise and
more
> efficient code.
>
> What would be your suggestion? To remove IsArrow, IsGenerator and
> IsConciseMethod and then do another masking as needed?
Yes. Just add some global helpers IsArrowKind, IsGeneratorKind and so
on if you
want to avoid bit fiddling everywhere.
Done.
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) {
On 2014/08/25 09:39:29, rossberg wrote:
Each of these functions should assert that kind is well-formed.
Done.
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) {
On 2014/08/25 09:39:30, rossberg wrote:
Assert that kind is well-formed.
Done.
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(
On 2014/09/02 14:42:48, arv wrote:
On 2014/08/25 09:39:30, rossberg wrote:
> Nit: Given that it's called only in one place, I don't think this
wrapper is
> necessary.
This was done done for readability. The out params are don't care and
since it
was being called from a context where we have is_getter and is_setter
it got
confusing.
Removed.
https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode1897
src/preparser.h:1897: checker->CheckProperty(next, kValueProperty,
On 2014/09/02 14:42:48, arv wrote:
On 2014/08/25 09:39:30, rossberg wrote:
> Can this check be factored out and moved after the if-chain, to
avoid its
> duplication?
I had that at first but the call to CheckProperty depends on the
current state
of the parser/scanner. I can try to refactor further.
Decided not to change this.
1. The duplicate check will go away in ES6.
2. We want to check the name before we parse the expression/function
body
3. The ObjectLiteralChecker depends on the current position of the
scanner.
https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode1903
src/preparser.h:1903: // Concise Method
On 2014/08/25 09:39:30, rossberg wrote:
Nit: V8 typically uses a style where a comment like this is placed
inside the
respective branch.
Done.
https://codereview.chromium.org/477263002/diff/160001/src/preparser.h#newcode1937
src/preparser.h:1937: LiteralT key = this->EmptyLiteral();
On 2014/08/25 09:39:30, rossberg wrote:
Nit: use ?: instead of `if` to avoid the redundant init.
Done.
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[] = {
On 2014/08/25 09:39:30, rossberg wrote:
Hm, well, it's not really object literals in there...
Done.
https://codereview.chromium.org/477263002/diff/160001/test/cctest/test-parsing.cc#newcode3414
test/cctest/test-parsing.cc:3414: {"'use strict'; ({", "(x, y) {}});"},
On 2014/08/25 09:39:30, rossberg wrote:
Good idea, you should probably add that to your other tests as well,
even though
it _should_ not matter there.
Done.
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.
