First round of comments.
https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc
File src/api.cc (right):
https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc#newcode958
src/api.cc:958: static void TemplateSet(i::Isolate* isolate,
I don't understand what this function is for... o_O
https://chromiumcodereview.appspot.com/22903012/diff/1/src/api.cc#newcode970
src/api.cc:970: if (data[i].IsEmpty()) {
Nit: Using a ternary ?: to calculate the value and uncoditionally adding
it makes things a bit clearer (i.e. the loop is actually a "map" in the
FP sense).
https://chromiumcodereview.appspot.com/22903012/diff/1/src/apinatives.js
File src/apinatives.js (right):
https://chromiumcodereview.appspot.com/22903012/diff/1/src/apinatives.js#newcode120
src/apinatives.js:120: %DefineJsAccessor(obj, properties, i);
Hmmm, this is somehow inconsistent with the 'length == 3' case above:
There we do the parsing of the properties array in JavaScript, here we
do it on the native side. Perhaps we should parse it in JavaScript in
both cases.
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects-inl.h
File src/objects-inl.h (right):
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects-inl.h#newcode4443
src/objects-inl.h:4443: ACCESSORS_TO_SMI(AccessorPair, flag,
kFlagOffset)
Can we rename this from plain "flag" to something more specific like
"access_flags"? (OK, this collides with the new method below, but you
get the idea.)
The name "flag" contains roughly as much information as "foo" or "hurz".
:-)
As discussed offline: What were the results of measuring the number of
AccessorPairs in Chrome on typical web sites/applications?
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc
File src/objects.cc (right):
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode516
src/objects.cc:516: } else if (obj->IsAccessorPair()) {
Just curious: Can we reach this code path with Foreign callbacks? Is it
OK that they are not handled at all? Depending on that, we might want to
ASSERT something here, but I am not sure what. ;-)
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode582
src/objects.cc:582: } else if (obj->IsAccessorPair()) {
Same here.
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode3375
src/objects.cc:3375: } else if (obj->IsAccessorPair()) {
... and here.
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.cc#newcode6213
src/objects.cc:6213: ASSERT_EQ(v8::DEFAULT, access_control);
Perhaps we should document this restriction in v8.h.
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.h
File src/objects.h (right):
https://chromiumcodereview.appspot.com/22903012/diff/1/src/objects.h#newcode9677
src/objects.h:9677: // * a pointer to a map: a transition used to
ensure map sharing
Extend the comment, covering the access flag part.
https://chromiumcodereview.appspot.com/22903012/
--
--
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/groups/opt_out.
