Large change. First batch of comments. :)
http://codereview.chromium.org/2101002/diff/2001/3003 File src/arm/ic-arm.cc (right): http://codereview.chromium.org/2101002/diff/2001/3003#newcode768 src/arm/ic-arm.cc:768: // r2: untagged index Update r2 comment. No longer holds untagged index at this point. http://codereview.chromium.org/2101002/diff/2001/3003#newcode787 src/arm/ic-arm.cc:787: // r2: untagged index Update r2 comment. No longer holds untagged index at this point. http://codereview.chromium.org/2101002/diff/2001/3007 File src/globals.h (right): http://codereview.chromium.org/2101002/diff/2001/3007#newcode536 src/globals.h:536: // MAP_POINTER_ALIGN returns the value aligned as a map pointer. Do you want to change the name of OBJECT_SIZE_ALIGN as well? This is the same operation for maps so the naming should be consistent. http://codereview.chromium.org/2101002/diff/2001/3008 File src/heap-inl.h (right): http://codereview.chromium.org/2101002/diff/2001/3008#newcode245 src/heap-inl.h:245: void Heap::CopyBlockToOldSpaceAndUpdateRegionsMarks(Address dst, RegionsMarks -> RegionMarks? http://codereview.chromium.org/2101002/diff/2001/3008#newcode280 src/heap-inl.h:280: Object** src_object_p = reinterpret_cast<Object**>(src); I don't like the _p suffix. We are not using that naming convention anywhere else. http://codereview.chromium.org/2101002/diff/2001/3008#newcode293 src/heap-inl.h:293: void Heap::MoveBlockToOldSpaceAndUpdateRegionsMarks(Address dst, RegionsMarks -> RegionMarks Also, isn't this method identical to CopyBlockToOldSpaceAndUpdateRegionMarks except for the second assert and the for/while difference? We should have only one of these or share the implementation? http://codereview.chromium.org/2101002/diff/2001/3009 File src/heap.cc (right): http://codereview.chromium.org/2101002/diff/2001/3009#newcode811 src/heap.cc:811: VerifyPageWatermarkValidity(old_pointer_space_, true); Use enum with values: VALID, INVALID instead of the boolean to make these call sites easier to read. http://codereview.chromium.org/2101002/diff/2001/3009#newcode1298 src/heap.cc:1298: obj = AllocateMap(BYTE_ARRAY_TYPE, ByteArray::kHeaderSize); This change looks a little bit spooky? All the other map allocations here use the aligned size of objects. Is this safe and if it is, could you add a comment to explain the difference? http://codereview.chromium.org/2101002/diff/2001/3009#newcode1578 src/heap.cc:1578: obj = AllocateSymbol(CStrVector(""), 0, 0); Moving from a named constant to an integer seems like a step in the wrong direction? http://codereview.chromium.org/2101002/diff/2001/3009#newcode3265 src/heap.cc:3265: ) { Indentation. http://codereview.chromium.org/2101002/diff/2001/3009#newcode3269 src/heap.cc:3269: Object** object_p = reinterpret_cast<Object**>(object_address); Remove _p suffix? http://codereview.chromium.org/2101002/diff/2001/3009#newcode3369 src/heap.cc:3369: ) { Indentation. Similar for the rest of the function in this file. http://codereview.chromium.org/2101002/diff/2001/3009#newcode3374 src/heap.cc:3374: Object** object_p = reinterpret_cast<Object**>(object_address); Remove _p suffix. Ditto for the rest of this file. http://codereview.chromium.org/2101002/diff/2001/3009#newcode3389 src/heap.cc:3389: return page + (((addr - page) + (Map::kSize - 1))/Map::kSize * Map::kSize); Space around operators: / http://codereview.chromium.org/2101002/diff/2001/3009#newcode3396 src/heap.cc:3396: return page + ((addr - page)/Map::kSize * Map::kSize); Space around binary operators: / http://codereview.chromium.org/2101002/diff/2001/3009#newcode3513 src/heap.cc:3513: return newmarks; Either use a one-liner or have braces around the body of the if. http://codereview.chromium.org/2101002/diff/2001/3010 File src/heap.h (right): http://codereview.chromium.org/2101002/diff/2001/3010#newcode757 src/heap.h:757: bool can_preallocate_during_iteration); Why is this called preallocate? Should it just be can_allocate_during_iteration? Also, would it make sense to have use an enum to get a meaningful name to use at callsites? http://codereview.chromium.org/2101002/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
