Please take another look
https://codereview.chromium.org/233293005/diff/100001/src/array.js
File src/array.js (right):
https://codereview.chromium.org/233293005/diff/100001/src/array.js#newcode475
src/array.js:475: %SetProperty(this, i+n, %_Arguments(i), 0, 0);
On 2014/04/11 13:12:08, Michael Starzinger wrote:
This will break for primitive values as "this" because there is not
ToObject
conversion anywhere and primitive values seem to be ignored by
Runtime_SetProperty.
Also this passes "sloppy" as the language mode, which causes this
[[Put]] not to
throw if the element cannot be put according to [[CanPut]].
Both of the above cases are already broken right now. So I am fine
with leaving
this as it is, but can we leave a TODO that this is a temporary
workaround? You
can assign this TODO to me if you want.
Done.
https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (left):
https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc#oldcode7663
src/hydrogen.cc:7663: if (receiver_map->instance_type() !=
JS_ARRAY_TYPE) return false;
On 2014/04/11 13:06:57, Toon Verwaest wrote:
This is invalid, since .push is spec'ed as reading the length, writing
the
element, updating the length.
1) regular objects don't have a HObjectAccess::ForArrayLength(...) and
2) the length may be observable through accessors.
Additionally, I presume UncheckedMono... doesn't actually do anything
to a
"length" field for non-JSArrays, so the fact that the length is
updated by .push
is relevant.
Please add tests to this effect.
Done.
https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/233293005/diff/100001/src/hydrogen.cc#newcode7666
src/hydrogen.cc:7666: if (!receiver_map->is_extensible()) return false;
On 2014/04/11 13:06:57, Toon Verwaest wrote:
I think is_extensible isn't necessary cause we guarantee we have
dictionary
elements for those cases. We should definitely ASSERT though.
Both these conditions should also hold for ArrayPop. In fact, they are
supposed
to hold for all BuildUncheckMonomorphicElementAccess users, so we
should ASSERT
them in that method.
I changed these into ASSERTs and added them to pop, too. Unfortuantely,
BuildUncheckMonomorphicElementAccess doesn't take a map so we can't use
it as a bottleneck.
https://codereview.chromium.org/233293005/
--
--
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.
