I didn't have time to do the refactoring of the runtime function yet. I'll
get
to it tomorrow.
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) {
On 2013/12/10 10:39:53, rossberg wrote:
Nit: perhaps call this FilterPublicSymbols?
Done.
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1045
src/v8natives.js:1045: if (IS_SYMBOL(propertyKeys[i]) &&
!%SymbolIsPrivate(propertyKeys[i])) {
On 2013/12/10 10:39:53, rossberg wrote:
It might make sense to introduce an IS_PRIVATE macro for the latter.
Done.
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.
On 2013/12/10 10:39:53, rossberg wrote:
Please use TODO(arv); same below
Done.
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1100
src/v8natives.js:1100: if (propertySet[symbol] === true) {
On 2013/12/10 10:39:53, rossberg wrote:
Could combine this with the previous if.
I also don't think this comment is correct. propertySet is an object
with a null [[Prototype]] so there is no toString on it. I'll update the
original case too.
https://codereview.chromium.org/108083005/diff/40001/src/v8natives.js#newcode1494
src/v8natives.js:1494: // getOwnPropertySymbols is added in symbol.js
On 2013/12/10 10:39:53, rossberg wrote:
Good to have that comment here. While you're at it, can you add the
same for
.observe?
Done.
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) {
On 2013/12/10 10:39:53, rossberg wrote:
Let's call this TestGetOwnPropertySymbols
Done.
https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/symbols.js#newcode305
test/mjsunit/harmony/symbols.js:305: assertEquals("symbol", typeof
syms[i])
On 2013/12/10 10:39:53, rossberg wrote:
Also assert that insertion order is maintained (which implies that
they are all
different).
You sent me off on goose chase there. I finally figured out that we only
use insertion order when %HasFastProperties is true
Done.
And please add a test that ensures that inherited symbols are not
included.
Done.
https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/symbols.js#newcode365
test/mjsunit/harmony/symbols.js:365: function
TestGetOwnPropertySymbolsWithPrivateSymbols() {
On 2013/12/10 10:39:53, rossberg wrote:
Nit: Move this next to the other test.
I kept it here because this test does not take an obj like the other
test. I also added one more test down here so it feels less out of
place.
https://codereview.chromium.org/108083005/diff/40001/test/mjsunit/harmony/symbols.js#newcode367
test/mjsunit/harmony/symbols.js:367: var obj = {}
On 2013/12/10 10:39:53, rossberg wrote:
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.
Doh!
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.
