DBC
http://codereview.chromium.org/7062004/diff/1/src/global-handles.cc File src/global-handles.cc (right): http://codereview.chromium.org/7062004/diff/1/src/global-handles.cc#newcode114 src/global-handles.cc:114: void GlobalHandles::Node::ClearWeakness(GlobalHandles* global_handles) { do we want to persist independence related state across MakeWeak/ClearWeakness calls, maybe they should reset independence flags? And shouldn't clearweakness move the handler out of the suffix as it's not collectible anymore? http://codereview.chromium.org/7062004/diff/1/src/global-handles.cc#newcode221 src/global-handles.cc:221: } else if (first_deallocated()) { just FYI: I recently thought about it, and chances really are we don't need both free and deallocated since we allocate the stuff from the pool anyway. I should have probably cleaned it up before. In any event it may compensate for newly added complexity. http://codereview.chromium.org/7062004/diff/1/src/global-handles.cc#newcode226 src/global-handles.cc:226: if (head() != NULL) head()->set_prev(result); nit: mixed style: it's (first_free()) in old code vs. if (head() != NULL), probably should be unified http://codereview.chromium.org/7062004/diff/1/src/global-handles.cc#newcode272 src/global-handles.cc:272: if (node->is_in_independent_tail_) { is it possible for this to be true? if node wasn't independent, it should not have belonged to independent tail unless there is a bug, no? http://codereview.chromium.org/7062004/diff/1/src/global-handles.cc#newcode347 src/global-handles.cc:347: if (!current->independent_) continue; shouldn't it always hold: ASSERT(current->independent_)? At least the name suggests that :) http://codereview.chromium.org/7062004/diff/1/src/global-handles.cc#newcode361 src/global-handles.cc:361: if (!current->independent_) continue; ditto http://codereview.chromium.org/7062004/diff/1/src/global-handles.cc#newcode420 src/global-handles.cc:420: node->is_in_independent_tail_ = false; is it mandatory here? apparently you overwrite it later anyway. http://codereview.chromium.org/7062004/diff/1/src/global-handles.h File src/global-handles.h (right): http://codereview.chromium.org/7062004/diff/1/src/global-handles.h#newcode355 src/global-handles.h:355: State state_ : 4; // Need one more bit for MSVC as it treats enums as signed. nit: apparently more than 80 chars (or my browser is wrong). http://codereview.chromium.org/7062004/diff/1/src/global-handles.h#newcode397 src/global-handles.h:397: void set_head(Node* value) { it might be more easier to reason about this code if you introduce reset_head which is called on tear down and assume that value != NULL in set_head itself http://codereview.chromium.org/7062004/diff/1/src/global-handles.h#newcode402 src/global-handles.h:402: !value->independent_) { given that set_head is invoked from ::Create, is it fine for value->independent_ to be true? And this flag should be reset by following Initialize, no? http://codereview.chromium.org/7062004/diff/1/src/global-handles.h#newcode411 src/global-handles.h:411: // This list forms a suffix of the whole list of nodes. in veryfyheap, can we assert this? or overall, do some more verification? http://codereview.chromium.org/7062004/diff/1/src/global-handles.h#newcode428 src/global-handles.h:428: nit: extra blank line? http://codereview.chromium.org/7062004/ -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev
