LGTM if comments are addressed.
https://codereview.chromium.org/203243004/diff/20001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/203243004/diff/20001/src/runtime.cc#newcode640
src/runtime.cc:640: HandleScope scope(isolate);
nit: Not that it makes much of a difference, but this can be within a
SealHandleScope here.
https://codereview.chromium.org/203243004/diff/20001/src/symbol.js
File src/symbol.js (right):
https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode66
src/symbol.js:66: function SymbolRegistry() {
nit: This naming makes it look like this is the implementation of
Symbol.registry, which doesn't exist. Could we instead rename it to
GetSymbolRegistry and move it up to around line 37 to clarify that it's
an internal helper?
https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode77
src/symbol.js:77: function SymbolHook(key) {
nit: Likewise for GetSymbolHook here.
https://codereview.chromium.org/203243004/diff/20001/src/symbol.js#newcode98
src/symbol.js:98: function SymbolKeyFor(symbol) {
Is it specified what "Symbol.keyFor(Symbol.create)" should return?
https://codereview.chromium.org/203243004/diff/20001/test/mjsunit/harmony/symbols.js
File test/mjsunit/harmony/symbols.js (right):
https://codereview.chromium.org/203243004/diff/20001/test/mjsunit/harmony/symbols.js#newcode414
test/mjsunit/harmony/symbols.js:414: assertFalse(Symbol.for("@@create")
=== Symbol.create)
On 2014/03/19 09:06:47, sof wrote:
Worth adding an initial test for keyFor() over well known symbols?
+1
https://codereview.chromium.org/203243004/
--
--
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.
