thanks, great comments =)
https://codereview.chromium.org/816913003/diff/240001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/816913003/diff/240001/src/code-stubs.h#newcode1618
src/code-stubs.h:1618: virtual CallInterfaceDescriptor
GetCallInterfaceDescriptor() OVERRIDE {
On 2015/01/21 16:26:28, arv wrote:
we no longer use virtual when override is present.
Acknowledged.
https://codereview.chromium.org/816913003/diff/240001/src/compiler/ast-graph-builder.cc
File src/compiler/ast-graph-builder.cc (right):
https://codereview.chromium.org/816913003/diff/240001/src/compiler/ast-graph-builder.cc#newcode2040
src/compiler/ast-graph-builder.cc:2040: // This hsould never lazy deopt,
so it is fine to send invalid bailout id.
On 2015/01/21 16:26:28, arv wrote:
typo
Acknowledged.
https://codereview.chromium.org/816913003/diff/240001/src/objects-inl.h
File src/objects-inl.h (right):
https://codereview.chromium.org/816913003/diff/240001/src/objects-inl.h#newcode6169
src/objects-inl.h:6169: return
shared()->scope_info()->IsSimpleParameterList();
On 2015/01/21 16:26:28, arv wrote:
or
shared()->is_simple_parameter_list();
Acknowledged.
https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-scopes.cc
File src/runtime/runtime-scopes.cc (right):
https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-scopes.cc#newcode513
src/runtime/runtime-scopes.cc:513: parameters -= rest_index;
On 2015/01/21 16:26:28, arv wrote:
I still think this would be cleaner if you didn't do pointer
arithmetics.
I think it's easier to read actually, otherwise it's like
`parameters[arithmetic producing a negative index]` which is really
confusing.
https://codereview.chromium.org/816913003/diff/240001/src/runtime/runtime-scopes.cc#newcode547
src/runtime/runtime-scopes.cc:547: // Determine parameter location on
the stack and dispatch on language mode.
On 2015/01/21 16:26:28, arv wrote:
" dispatch on language mode"
What does that mean?
The frame iterator stuff was copied from NewArguments (along with the
comment), it's talking about strict mode --- but yeah it doesn't really
apply here
https://codereview.chromium.org/816913003/diff/240001/src/scopes.cc
File src/scopes.cc (right):
https://codereview.chromium.org/816913003/diff/240001/src/scopes.cc#newcode1309
src/scopes.cc:1309: if (rest_parameter_ &&
!MustAllocate(rest_parameter_)) {
On 2015/01/21 16:26:28, arv wrote:
I'm curious. What is going on here?
MustAllocate() will return true if eval() is used, or if the variable is
bound to a VariableProxy, which I believe should happen whenever it is
referenced somewhere in the scope (but I may be wrong about that). So
the idea is, if it doesn't get used, don't allocate it.
This is part of the "there is a distinction between "rest arguments
used" and "rest arguments in formal parameters" --- there probably
doesn't need to be such a distinction, and working around it is kind of
awkward" that I mentioned in the comment.
It might not be worth worrying about this yet.
https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):
https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64.cc#newcode867
src/x64/code-stubs-x64.cc:867: // Patch the arguments.length and the
parameters pointer.
On 2015/01/21 16:26:28, arv wrote:
Maybe we should refactor this to not duplicate this code?
it would be nice, but it's hard as there's no guarantee the arguments
access stub was used/will be used
https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64.cc#newcode873
src/x64/code-stubs-x64.cc:873:
StandardFrameConstants::kCallerSPOffset));
On 2015/01/21 16:26:28, arv wrote:
wrong indentation
Acknowledged.
https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64.cc#newcode883
src/x64/code-stubs-x64.cc:883: /*(__ movp(rdx, rcx);
On 2015/01/21 16:26:28, arv wrote:
Remove?
I was trying to figure out a way to allocate and set up the maps from
here, but yeah for now I guess it can just always call runtime.
https://codereview.chromium.org/816913003/diff/240001/src/x64/code-stubs-x64.cc#newcode894
src/x64/code-stubs-x64.cc:894: // __ Allocate(rcx, rax, rdx, rbx,
&runtime, TAG_OBJECT);
On 2015/01/21 16:26:28, arv wrote:
remove?
Acknowledged.
https://codereview.chromium.org/816913003/diff/240001/src/x64/full-codegen-x64.cc
File src/x64/full-codegen-x64.cc (right):
https://codereview.chromium.org/816913003/diff/240001/src/x64/full-codegen-x64.cc#newcode251
src/x64/full-codegen-x64.cc:251: Operand(rbp,
StandardFrameConstants::kCallerSPOffset + offset));
On 2015/01/21 16:26:28, arv wrote:
indentation
Acknowledged.
https://codereview.chromium.org/816913003/diff/240001/test/mjsunit/harmony/rest-params.js
File test/mjsunit/harmony/rest-params.js (right):
https://codereview.chromium.org/816913003/diff/240001/test/mjsunit/harmony/rest-params.js#newcode40
test/mjsunit/harmony/rest-params.js:40: function sloppySlowTest(a, a,
...c) {
On 2015/01/21 16:26:28, arv wrote:
This should be a SyntaxError.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-function-definitions-static-semantics-early-errors
Maybe file a bug so we don't forget about this?
hmm you're right. It should be a syntax error, I'll fix it up in this CL
https://codereview.chromium.org/816913003/
--
--
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.
