LGTM, just a couple of nits to address.
https://codereview.chromium.org/196103004/diff/40001/src/api.cc
File src/api.cc (right):
https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6154
src/api.cc:6154: i::Handle<i::String> part =
i::handle(i_isolate->heap()->for_string());
nit: Just use i_isolate->factory()->for_string() instead.
https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6172
src/api.cc:6172: i::Handle<i::String> part =
i::handle(i_isolate->heap()->for_api_string());
nit: Just use i_isolate->factory()->for_api_string() instead.
https://codereview.chromium.org/196103004/diff/40001/src/api.cc#newcode6203
src/api.cc:6203: i::handle(i_isolate->heap()->private_api_string());
nit: Just use i_isolate->factory()->private_api_string() instead.
https://codereview.chromium.org/196103004/diff/40001/src/isolate.cc
File src/isolate.cc (right):
https://codereview.chromium.org/196103004/diff/40001/src/isolate.cc#newcode2348
src/isolate.cc:2348: return
handle(JSObject::cast(heap()->symbol_registry()));
nit: Better use "return
Handle<JSObject>::cast(factory()->symbol_registry())".
https://codereview.chromium.org/196103004/diff/40001/src/isolate.h
File src/isolate.h (right):
https://codereview.chromium.org/196103004/diff/40001/src/isolate.h#newcode1129
src/isolate.h:1129: Handle<JSObject> GetSymbolRegistry();
nit: Can we add a short one-line comment of the intended use?
https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc#newcode629
src/runtime.cc:629: Handle<String> part =
handle(isolate->heap()->private_intern_string());
nit: You can just use isolate->factory()->private_intern_string() here.
https://codereview.chromium.org/196103004/diff/40001/src/runtime.cc#newcode659
src/runtime.cc:659: HandleScope hs(isolate);
nit: s/hs/scope/ for consistency and grep-ability.
https://codereview.chromium.org/196103004/diff/40001/src/symbol.js
File src/symbol.js (right):
https://codereview.chromium.org/196103004/diff/40001/src/symbol.js#newcode67
src/symbol.js:67: var registry = %SymbolRegistry().for_intern;
nit: The usage of the local variable "registry" is different here and
below, this is a little bit confusing IMHO.
https://codereview.chromium.org/196103004/
--
--
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.
