https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js
File src/v8natives.js (right):
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1041
src/v8natives.js:1041: function GetSymbolsInArray(propertyKeys) {
Nit: perhaps call this FilterPublicSymbols?
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1045
src/v8natives.js:1045: if (IS_SYMBOL(propertyKeys[i]) &&
!%SymbolIsPrivate(propertyKeys[i])) {
It might make sense to introduce an IS_PRIVATE macro for the latter.
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1060
src/v8natives.js:1060: // FIXME: Proxies use a shared trap for String
and Symbol keys.
Please use TODO(arv); same below
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1072
src/v8natives.js:1072: // FIXME: Have the runtime do the filtering.
Yeah, I would very much prefer to have this implemented efficiently
right away, at least for ordinary properties (for interceptor properties
it matters less). Just turn the Boolean flag into a three-way flag.
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1100
src/v8natives.js:1100: if (propertySet[symbol] === true) {
Could combine this with the previous if.
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1494
src/v8natives.js:1494: // getOwnPropertySymbols is added in symbol.js
Good to have that comment here. While you're at it, can you add the same
for .observe?
https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/symbols.js
File test/mjsunit/harmony/symbols.js (right):
https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/symbols.js#newcode301
test/mjsunit/harmony/symbols.js:301: function TestKeySymbols(obj) {
Let's call this TestGetOwnPropertySymbols
https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/symbols.js#newcode305
test/mjsunit/harmony/symbols.js:305: assertEquals("symbol", typeof
syms[i])
Also assert that insertion order is maintained (which implies that they
are all different).
And please add a test that ensures that inherited symbols are not
included.
https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/symbols.js#newcode365
test/mjsunit/harmony/symbols.js:365: function
TestGetOwnPropertySymbolsWithPrivateSymbols() {
Nit: Move this next to the other test.
https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/symbols.js#newcode367
test/mjsunit/harmony/symbols.js:367: var obj = {}
Hm, the private symbol is never added to obj. ;)
Also, maybe have a mixture of both public and private symbols, and check
that insertion order still holds after non-trivial filtering.
https://codereview.chromium.org/108083005/
--
--
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.
