On 2015/02/25 14:49:09, caitp wrote:
Thanks
https://codereview.chromium.org/938443002/diff/80001/src/parser.cc
File src/parser.cc (right):
https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5497
src/parser.cc:5497: int i = 0;
On 2015/02/25 14:28:18, rossberg wrote:
> On 2015/02/23 17:55:56, arv wrote:
> > This could use a comment explaining what it desugars to.
>
> +1
Acknowledged.
https://codereview.chromium.org/938443002/diff/80001/src/parser.cc#newcode5508
src/parser.cc:5508: args->Add(factory()->NewArrayLiteral(unspread,
literal_index,
On 2015/02/23 17:55:56, arv wrote:
> Will this be observable if someone overrides
Array.prototoype[Symbol.iterator]?
The unspread array literals aren't iterated via iterator protocol, so no.
Could
be observable if the length property were defined though.
https://codereview.chromium.org/938443002/diff/160001/src/preparser.h
File src/preparser.h (right):
https://codereview.chromium.org/938443002/diff/160001/src/preparser.h#newcode2358
src/preparser.h:2358: int unspread_sequences_count = 0;
On 2015/02/25 14:28:18, rossberg wrote:
> A brief comment explaining what this actually counts would be useful. I
gather
> it's the number of non-empty sequences of non-spread arguments?
That's right --- will add a comment
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-function.cc
File src/runtime/runtime-function.cc (right):
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-function.cc#newcode641
src/runtime/runtime-function.cc:641: // TODO(caitp): Support larger
element
indexes (up to 2^53-1).
On 2015/02/25 14:28:19, rossberg wrote:
> Ah, no. We likely won't, ever, at least not for calls -- you will never
have
a
> stack that large anyway.
sorry, that's copy/pasted from similar code I wrote in ArrayConcat, where
the
"length" property is spec'd to support up to 2^53-1. I agree the comment
doesn't
really apply here.
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-function.cc#newcode643
src/runtime/runtime-function.cc:643:
ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate,
val,
On 2015/02/25 14:28:19, rossberg wrote:
> I think it's more adequate to raise an exception here. RangeError or
OOM.
The only reason ToLength() would throw is if the value is an object with a
custom toString() or valueOf() which throws, iirc. So, it wouldn't really
be
representative of OOM or RangeError
There isn't really a good reason to use ToLength() here anyways, though.
The
arguments object should always be a JSArray, so I guess I'll just remove
the
else-case here.
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime-function.cc#newcode682
src/runtime/runtime-function.cc:682: if (!fun->IsJSFunction()) {
On 2015/02/25 14:28:19, rossberg wrote:
> In a similar vein, this logic belongs into Execution::New.
The problem with letting Execution::New() and Execution::Call() do it, is
you
end up with an illegal instruction if they don't find something callable.
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime.h
File src/runtime/runtime.h (right):
https://codereview.chromium.org/938443002/diff/160001/src/runtime/runtime.h#newcode62
src/runtime/runtime.h:62: F(ApplyCall, 3, 1) \
On 2015/02/25 14:28:19, rossberg wrote:
> How about slightly more descriptive names? E.g. ApplyAll and
ApplyConstructAll.
Acknowledged.
I've changed this to use ReflectApply / ReflectConstruct rather than the new
runtime functions (which can be deleted if that's cool) --- it means
newTarget
stuff works correctly-ish
https://codereview.chromium.org/938443002/
--
--
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.
