First round of drive-by comments.
https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc File src/mark-compact.cc (right): https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#newcode1910 src/mark-compact.cc:1910: base_marker()->MarkObjectAndPush(reinterpret_cast<HeapObject*>(key)); We can also use HeapObject::cast() here. https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#newcode1965 src/mark-compact.cc:1965: base_marker()->MarkObjectAndPush(reinterpret_cast<HeapObject*>(key)); Use HeapObject::cast() here. https://chromiumcodereview.appspot.com/10697015/diff/1/src/mark-compact.cc#newcode1978 src/mark-compact.cc:1978: base_marker()->MarkObjectWithoutPush(value); We only need to mark the slots if the pair itself wasn't marked. So move the MarkObjectWithoutPush() into the condition. https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc File src/objects-debug.cc (right): https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#newcode921 src/objects-debug.cc:921: if (!IsTransitionArray()) return true; Can this really happen? If not, drop this line. https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#newcode948 src/objects-debug.cc:948: if (this == NULL) return true; Can "this" really be NULL? If not, drop this line. https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#newcode956 src/objects-debug.cc:956: continue; Drop the continue. https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-debug.cc#newcode958 src/objects-debug.cc:958: if (!CheckOneBackPointer(current_map, value)) return false; Can we turn this into a proper "else" branch and also assert that it actually is a map in the else branch? https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h File src/objects-inl.h (right): https://chromiumcodereview.appspot.com/10697015/diff/1/src/objects-inl.h#newcode2001 src/objects-inl.h:2001: if (!this->MayContainTransitions()) return NULL; Somehow it feels wrong to return NULL here. Either return some empty transitions array (e.g. EmptyFixedArray) or make the return type of this method "Object" and return Smi::FromInt(0) here. https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h File src/transitions.h (right): https://chromiumcodereview.appspot.com/10697015/diff/1/src/transitions.h#newcode49 src/transitions.h:49: inline Map* elements(); Can we call this elements_transition instead? https://chromiumcodereview.appspot.com/10697015/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
