On Tue, Nov 15, 2011 at 10:34 AM, <[email protected]> wrote: > 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.
Have you tried compiling it with arch=x64? I understand that you want to lose the high 32 bits in this case. >> 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. :( This is exactly why we have to call _exit in shell and d8. >> 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++. No, it's not. reinterpret_cast requires the same bit width. >> 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. C'mon, is it too much trouble to do this right? > >> 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. Thank you. You understand that nobody is going to clean this up, don't you? >> 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
