LGTM with a bunch of nits.
http://codereview.chromium.org/335009/diff/1/20 File src/heap.h (right): http://codereview.chromium.org/335009/diff/1/20#newcode307 Line 307: static bool linear_allocation() { Move this one after the other always_allocate_* getter? http://codereview.chromium.org/335009/diff/1/16 File src/serialize.cc (right): http://codereview.chromium.org/335009/diff/1/16#newcode1444 Line 1444: GlobalHandles::TearDown(); Shouldn't we be checking that there are no global handles at this point? http://codereview.chromium.org/335009/diff/1/16#newcode1758 Line 1758: : source_(source), Four space indent. http://codereview.chromium.org/335009/diff/1/16#newcode1792 Line 1792: new_page = (old_fullness >> Page::kPageSizeBits) + 1; current_page + 1? http://codereview.chromium.org/335009/diff/1/16#newcode1802 Line 1802: Page::kPageAlignmentMask) == Funky indentation. I would do: ASSERT((reinterpret_cast<intptr_t>(new_object->address()) & Page::kPageAlignmentMask) == ((old_fullness & Page::kPageAlignmentMask) + Page::kObjectStartOffset)); http://codereview.chromium.org/335009/diff/1/16#newcode1899 Line 1899: // returns a true of the new object was in young space and false otherwise. returns true if the new ... http://codereview.chromium.org/335009/diff/1/16#newcode2017 Line 2017: : sink_(sink), Four space indent. http://codereview.chromium.org/335009/diff/1/7 File src/serialize.h (right): http://codereview.chromium.org/335009/diff/1/7#newcode356 Line 356: SnapshotByteSource(const byte* array, int length) Please add spaces between the methods in this class. http://codereview.chromium.org/335009/diff/1/7#newcode377 Line 377: return accumulator; Put an UNREACHABLE() in front of this return as documentation? Is all of this getting inlined? Maybe have the fast case here and the rest in the implementation file? http://codereview.chromium.org/335009/diff/1/7#newcode405 Line 405: static const int kSmiBias = 16; Could you add a comment about the bias? http://codereview.chromium.org/335009/diff/1/7#newcode422 Line 422: // Create a deserializer. The snapshot is held in str and has size len. Create a deserializer from a byte source? The rest of the comment looks out-dated. http://codereview.chromium.org/335009/diff/1/7#newcode436 Line 436: virtual void VisitExternalReferences(Address* start, Address* end) { I would add space before and after the methods that do not fit on a single line for readability. http://codereview.chromium.org/335009/diff/1/7#newcode477 Line 477: const int max_shift = ((kPointerSize * kBitsPerByte) / 7) * 7; Move to implementation file? http://codereview.chromium.org/335009/diff/1/7#newcode495 Line 495: void Finalize(int* y) {} Add a comment? What does Finalize do? What is y? http://codereview.chromium.org/335009/diff/1/7#newcode504 Line 504: TAGGED_REPRESENTATION, // Deserialize as a tagged object reference. How about just "Tagged object reference."? http://codereview.chromium.org/335009/diff/1/7#newcode505 Line 505: CODE_TARGET_REPRESENTATION // Deserialize a reference to first instruction. How about just "Reference to first instruction in code target."? http://codereview.chromium.org/335009/diff/1/15 File src/spaces.cc (right): http://codereview.chromium.org/335009/diff/1/15#newcode1834 Line 1834: // There is no next page in this space. Try free list allocation. Update comment. http://codereview.chromium.org/335009/diff/1/15#newcode2238 Line 2238: // There is no next page in this space. Try free list allocation. Update comment. http://codereview.chromium.org/335009/diff/1/2 File test/cctest/test-serialize.cc (right): http://codereview.chromium.org/335009/diff/1/2#newcode190 Line 190: // We have to create one context. One reason for this is that the One reason for this is so that the builtins ... http://codereview.chromium.org/335009 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
