Thanks Toon!
On 2014/08/20 07:43:11, Toon Verwaest wrote:
Added comments for the code, still need to look through the tests.
What about function proxies btw?
I plan to just fail on them for now. Our implementation of proxies is not
functional anyway.
Let me know if you think it is an issue for production code as well.
https://codereview.chromium.org/475423003/diff/100001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/475423003/diff/100001/src/objects.cc#newcode9415
src/objects.cc:9415: Handle<FixedArrayBase>
original_elements(function->elements());
Will do.
Use the ElementsAccessor Copy method instead.
https://codereview.chromium.org/475423003/diff/100001/src/objects.cc#newcode9433
src/objects.cc:9433: if (details.type() == NORMAL || details.type() ==
FIELD)
{
NORMAL is not a valid entry for descriptors. Only FIELD needs to be
copied.
We should better separate those ...
That also indicates though that if your function is in dictionary mode,
you
don't copy anything from the backing store. Hence you'll lose all your
properties. For fast mode you copy them. Please add a test that
normalizes the
function before binding. (The runtime function to turn something into
dictionary
mode is called %OptimizeObjectForAddingMultipleProperties).
What about having separate methods to copy named and indexed properties,
extracted from the regular object copying mechanism?
Thanks, and right, we should share the code between this function and
Heap::CopyJSObject.
AFAIU Heap::CopyJSObject has the same bug, i.e. it will shared the
properties
dictionary between the source and the copy instead of copying it.
Is there any other object copying mechanism?
https://codereview.chromium.org/475423003/diff/100001/src/property.h
File src/property.h (right):
https://codereview.chromium.org/475423003/diff/100001/src/property.h#newcode26
src/property.h:26: Descriptor() : details_(Smi::FromInt(0)) {}
Is this change necessary?
No, a stray change.
https://codereview.chromium.org/475423003/diff/100001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/475423003/diff/100001/src/runtime.cc#newcode2064
src/runtime.cc:2064: DONT_SHOW).Assert();
DONT_ENUM.
The other flags are just there for filtering. Probably we should extract
them
to
a separate filter class rather than having them as part of the regular
PropertyAttributes. We don't even have enough bits to encode DONT_SHOW
into
the
descriptor I believe.
Will fix, thanks.
https://codereview.chromium.org/475423003/
--
--
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.
