I couldn't get rid of a feeling that this is really a few independent
changes
put together in one patch. I'm not against huge patches that make sense,
i.e.
make logical steps forward, but huge patches like this one risk decreasing
the
quality of reviews. Once again let me remind you of git svn that makes
managing
such changes way easier.
http://codereview.chromium.org/2841023/diff/21002/34004
File src/api.cc (right):
http://codereview.chromium.org/2841023/diff/21002/34004#newcode2818
src/api.cc:2818: class StringTracker {
This should become a part of the Isolate. Keeping a static interface to
a per-isolate state is something we try to avoid. Move the declaration
to api.h or create string-tracker.{h,cc}.
http://codereview.chromium.org/2841023/diff/21002/34008
File src/debug.cc (right):
http://codereview.chromium.org/2841023/diff/21002/34008#newcode805
src/debug.cc:805: Isolate::Current()->set_context(*context);
Change occurrences of "Isolate::Current()" to "isolate" in this
function's scope.
http://codereview.chromium.org/2841023/diff/21002/34008#newcode1886
src/debug.cc:1886: Debugger::~Debugger() {
We should free the mutexes.
http://codereview.chromium.org/2841023/diff/21002/34012
File src/global-handles.cc (right):
http://codereview.chromium.org/2841023/diff/21002/34012#newcode216
src/global-handles.cc:216: GlobalHandles* global_handles_;
Storing the pointer in every created global handle is wasteful. Try
passing it to the functions that need the pointer instead.
http://codereview.chromium.org/2841023/diff/21002/34013
File src/global-handles.h (right):
http://codereview.chromium.org/2841023/diff/21002/34013#newcode185
src/global-handles.h:185: };
Missing DISALLOW_COPY_AND_ASSIGN.
http://codereview.chromium.org/2841023/diff/21002/34016
File src/heap.cc (right):
http://codereview.chromium.org/2841023/diff/21002/34016#newcode4484
src/heap.cc:4484: void Heap::PreInit() {
Are there any other planned usages of PreInit? If not, make debug utils
initialization a part of the constructor.
http://codereview.chromium.org/2841023/diff/21002/34018
File src/isolate.cc (right):
http://codereview.chromium.org/2841023/diff/21002/34018#newcode516
src/isolate.cc:516: const char* Isolate::GetVersion() {
Surely all our isolates are going to have a single version. Make this
buffer statically initialized in version.cc and move GetVersion() there.
http://codereview.chromium.org/2841023/diff/21002/34019
File src/isolate.h (right):
http://codereview.chromium.org/2841023/diff/21002/34019#newcode782
src/isolate.h:782: NativeAllocationChecker::NativeAllocationChecker(
Move these to allocation-inl.h.
http://codereview.chromium.org/2841023/diff/21002/34021
File src/log.cc (right):
http://codereview.chromium.org/2841023/diff/21002/34021#newcode311
src/log.cc:311: if (Logger::profiler_) {
I asked Mikhail to have a look.
http://codereview.chromium.org/2841023/diff/21002/34030
File src/v8threads.cc (right):
http://codereview.chromium.org/2841023/diff/21002/34030#newcode279
src/v8threads.cc:279: // TODO(isolates): Destroy TLS keys.
... and mutexes.
http://codereview.chromium.org/2841023/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
