First round of comments, mostly just style comments.
http://codereview.chromium.org/1217011/diff/8001/9001 File src/heap.cc (right): http://codereview.chromium.org/1217011/diff/8001/9001#newcode864 src/heap.cc:864: return 0; Style issue: we write NULL instead of 0 for pointers. http://codereview.chromium.org/1217011/diff/8001/9001#newcode884 src/heap.cc:884: String *target = updater_func(p); Style issue: the star goes next to "String" not "target". http://codereview.chromium.org/1217011/diff/8001/9001#newcode886 src/heap.cc:886: if (target == 0) { NULL not 0. Style issue: for single line bodies you are welcome to put them on the same line as the if with no braces. I think that's more readable here: if (target == NULL) continue; http://codereview.chromium.org/1217011/diff/8001/9004 File src/mark-compact.cc (right): http://codereview.chromium.org/1217011/diff/8001/9004#newcode52 src/mark-compact.cc:52: static inline void MoveBlock(Address dst, Address src, size_t n) { You might consider making this a static function on Heap (and move it into heap-inl.h) to go with Heap::CopyBlock. "n" is not a good name for the argument. Something like "size_in_bytes" or something that indicates the dimensions is better. http://codereview.chromium.org/1217011/diff/8001/9004#newcode54 src/mark-compact.cc:54: ASSERT(IsAligned<size_t>(n, kPointerSize)); Lift this ASSERT outside the if? http://codereview.chromium.org/1217011/diff/8001/9004#newcode57 src/mark-compact.cc:57: Address* p = reinterpret_cast<Address*>(dst); This is not an Address*. Object**? http://codereview.chromium.org/1217011/diff/8001/9004#newcode61 src/mark-compact.cc:61: while (q != end) The style rule is that if the body is one line and on the same line as the while, you can leave off the braces. If the body begins on the next line, you need the braces. http://codereview.chromium.org/1217011/diff/8001/9004#newcode63 src/mark-compact.cc:63: } else if (dst >= (src + n)) { Isn't this case the same as the previous one? It seems like the two cases should just be combined. http://codereview.chromium.org/1217011/diff/8001/9004#newcode1101 src/mark-compact.cc:1101: // First pass migrates all alive objects from one semispace to another or "The first pass"..."promotes". "Forwading addresses a" ==> "Forwarding addresses are". Written directly into what? "The second pass". http://codereview.chromium.org/1217011/diff/8001/9004#newcode1107 src/mark-compact.cc:1107: // map space sweeping is done after new space sweeping. This comment might be clarified to mention that we sweep the remembered sets in the map space. http://codereview.chromium.org/1217011/diff/8001/9004#newcode1118 src/mark-compact.cc:1118: // It does not expect to encounter pointers to dead objects. Extra space between "to" and "encounter". http://codereview.chromium.org/1217011/diff/8001/9004#newcode1134 src/mark-compact.cc:1134: reinterpret_cast<Code*>(target)->instruction_start()); If you use reinterpret_cast instead of Code::cast because the object is marked, say so in a short comment so the reader doesn't wonder. http://codereview.chromium.org/1217011/diff/8001/9004#newcode1172 src/mark-compact.cc:1172: if (new_addr == 0) return; NULL. http://codereview.chromium.org/1217011/diff/8001/9004#newcode1249 src/mark-compact.cc:1249: Object *target = space->AllocateRaw(size); Star goes next to Object not target. http://codereview.chromium.org/1217011/diff/8001/9004#newcode1257 src/mark-compact.cc:1257: Memory::Address_at(current) = 0; NULL. http://codereview.chromium.org/1217011/diff/8001/9004#newcode1261 src/mark-compact.cc:1261: // Second Pass: find pointers to new space and update them. "Pass" ==> "pass". http://codereview.chromium.org/1217011/diff/8001/9004#newcode1264 src/mark-compact.cc:1264: // Update pointers in new space. Maybe "new space" ==> "to space".... http://codereview.chromium.org/1217011/diff/8001/9004#newcode1265 src/mark-compact.cc:1265: HeapObject *object; Star goes next to HeapObject. http://codereview.chromium.org/1217011/diff/8001/9004#newcode1287 src/mark-compact.cc:1287: cell != NULL; cell = cell_iterator.next()) { I think you should be each part of the for loop header on their own line, unless they fit on a single line. http://codereview.chromium.org/1217011 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev To unsubscribe from this group, send email to v8-dev+unsubscribegooglegroups.com or reply to this email with the words "REMOVE ME" as the subject.
