Some replies to the comments.
I'll address the ones I didn't reply to.
On 2011/11/15 18:14:17, Vitaly Repeshko wrote:
http://codereview.chromium.org/8536042/diff/1/src/api.cc
File src/api.cc (right):
http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode505
src/api.cc:505:
nit: Remove one blank line.
http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode509
src/api.cc:509: reinterpret_cast<uint32_t>(extension));
I'm not sure it'll work with 64 bit pointers. I think you need something
like
static_cast<uint32_t>(reinterpret_cast<uintptr_t>(extension)).
I just need a hash value, and it does not have to be a perfect hash, so
it's all
good.
http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode517
src/api.cc:517: static Allocator default_allocator;
Allocate the allocator with "new" to avoid issues with static destructors.
Allocator is BASE_EMBEDDED, so can't use new.
http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode529
src/api.cc:529: entry = Lookup(extension, Hash(extension), true);
Can't you simply return UNVISITED?
http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode534
src/api.cc:534: reinterpret_cast<int>(entry->value));
Again, I think it has to be something like
static_cast<ExtensionTraversalState>(reintepret_cast<intptr_t>(entry->value)).
cast from void* to int is always ok in C++.
http://codereview.chromium.org/8536042/diff/1/src/api.cc#newcode542
src/api.cc:542: }
nit: // namespace internal
http://codereview.chromium.org/8536042/diff/1/src/api.h
File src/api.h (right):
http://codereview.chromium.org/8536042/diff/1/src/api.h#newcode164
src/api.h:164: class ExtensionStates : private HashMap {
Use delegation instead of implementation inheritance.
All other uses of HashMap in V8 use implementation inheritance.
http://codereview.chromium.org/8536042/diff/1/src/api.h#newcode170
src/api.h:170: }
nit: // namespace internal
http://codereview.chromium.org/8536042/diff/1/src/isolate.h
File src/isolate.h (right):
http://codereview.chromium.org/8536042/diff/1/src/isolate.h#newcode814
src/isolate.h:814: ExtensionStates* extension_states() { return
extension_states_; }
It's really unfortunate that this is a part of the isolate state. Please
explore
a slightly different approach. Make the InstallExtension family of
functions
instance (instead of static) methods of Genesis and move all the extension
state
tracking code there. To support the assert in serialize.cc a simple bool
flag
in
Bootstrapper (e.g. "have_installed_extensions") should be enough.
Yes this is a good idea - another approach would be to just have an
ExtensionStates parameter to InstallExtensions, so as not to pollute
Genesis.
http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc
File test/cctest/test-lockers.cc (right):
http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc#newcode662
test/cctest/test-lockers.cc:662: context.Dispose();
Dispose the isolate too?
http://codereview.chromium.org/8536042/diff/1/test/cctest/test-lockers.cc#newcode669
test/cctest/test-lockers.cc:669: TEST(ExtensionsRegistration) {
Please describe what this tests and add a link to the bug.
http://codereview.chromium.org/8536042/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
