LGTM
http://codereview.chromium.org/2069013/diff/1/2 File src/serialize.cc (right): http://codereview.chromium.org/2069013/diff/1/2#newcode703 src/serialize.cc:703: COMMON_RAW_LENGTHS(RAW_CASE) How about moving all the cases to after the macro definitions? That is move COMMON_RAW_LENGTHS(RAW_CASE) case kRawData: { ... } down to just before ONE_PER_SPACE(kNewObject, kPlain, kStartOfObject) http://codereview.chromium.org/2069013/diff/1/2#newcode715 src/serialize.cc:715: case where + how + within + space_number: How about adding ASSERT((where & how & within & space_number) == 0); here? http://codereview.chromium.org/2069013/diff/1/2#newcode762 src/serialize.cc:762: new_object = reinterpret_cast<Object*>( \ This is not an object pointer any more. http://codereview.chromium.org/2069013/diff/1/2#newcode807 src/serialize.cc:807: #define COMPACT_ONE_PER_SPACE(where, how, within) \ Can we find a better name that COMPACT_ONE_PER_SPACE? It treats new space specially and all other spaces in the same way, right? http://codereview.chromium.org/2069013/diff/1/2#newcode825 src/serialize.cc:825: The indentation reveals that something is going on here. How about adding a comment about it? Maybe even have some more comments between some of the the macro usages. http://codereview.chromium.org/2069013/diff/1/3 File src/serialize.h (right): http://codereview.chromium.org/2069013/diff/1/3#newcode140 src/serialize.h:140: // It is very common to have a reference to the object at word 11 in space 2, Please change comment to just indicate that these objects have been found by looking at the most pointed to objects at some point in time. http://codereview.chromium.org/2069013/diff/1/3#newcode201 src/serialize.h:201: static const int kPointerEncodingMask = 0x40; How about having more similarity between the name kPointerEncodingMask and enum HowToCode. kHowToCodeMask and enum HowToCode kPointerEncodingMask and enum PointerEncoding Also how about having the mask in the enum? http://codereview.chromium.org/2069013/diff/1/3#newcode209 src/serialize.h:209: static const int kPointerOffsetMask = 0x80; Ditto. http://codereview.chromium.org/2069013/diff/1/3#newcode239 src/serialize.h:239: Can we have static assets for kNumberOfSpaces == 9 and no overlap of the kPointedToMask, kPointerEncodingMask and kPointerOffsetMask? http://codereview.chromium.org/2069013/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
