LGTM with some comments.
http://codereview.chromium.org/10918067/diff/1/src/serialize.cc File src/serialize.cc (right): http://codereview.chromium.org/10918067/diff/1/src/serialize.cc#newcode611 src/serialize.cc:611: isolate_->heap()->ReserveSpace(&reservations_[0], &high_water_[0]); why dereference and re-reference, and not simply: ReserveSpace(reservations_, high_water_) unless you want to make sure it is obvious that we are dealing with arrays? http://codereview.chromium.org/10918067/diff/1/src/serialize.cc#newcode644 src/serialize.cc:644: isolate_->heap()->ReserveSpace(&reservations_[0], &high_water_[0]); Ditto. http://codereview.chromium.org/10918067/diff/1/src/serialize.cc#newcode750 src/serialize.cc:750: current) + skip); \ It seems easier readable to use Address instead of intptr_t. http://codereview.chromium.org/10918067/diff/1/src/serialize.cc#newcode762 src/serialize.cc:762: reinterpret_cast<intptr_t>(current) + skip); \ Ditto. http://codereview.chromium.org/10918067/diff/1/src/serialize.cc#newcode1000 src/serialize.cc:1000: ASSERT(integer <= 0x3fffff); I'd prefer integer < (1 << 22) http://codereview.chromium.org/10918067/diff/1/src/serialize.cc#newcode1065 src/serialize.cc:1065: sink_->Put(kRawData + kPointerSize / 4, "Smi"); The 4 in here obviously refers to the factor 4 in COMMON_RAW_LENGTH. Can we somehow make a constant out of this? http://codereview.chromium.org/10918067/diff/1/src/serialize.h File src/serialize.h (right): http://codereview.chromium.org/10918067/diff/1/src/serialize.h#newcode177 src/serialize.h:177: answer = *reinterpret_cast<const int32_t*>(data_ + position_); Since GetInt() uses this: we assume the least significant two bits to tell how many bytes are part of the integer, so the byte order matters. This would only work on little endian systems, right? The #else block seems to work for big endian as well. I don't think we have any big endian system that reads unaligned, but an assertion won't hurt? http://codereview.chromium.org/10918067/diff/1/src/serialize.h#newcode182 src/serialize.h:182: answer |= data_[position_ + 3] << 24; In PutInt we only take integers that can be encoded in 3 bytes. Why even fetch a 4th byte? http://codereview.chromium.org/10918067/diff/1/src/serialize.h#newcode235 src/serialize.h:235: f(31, 124) if the second arg is always 4x the first arg, why not make that part of f? Or at least add a few STATIC_CHECK in some of the f. http://codereview.chromium.org/10918067/diff/1/src/serialize.h#newcode546 src/serialize.h:546: code_object_ = o->IsCode(); I guess this could also be put into the initialization list. http://codereview.chromium.org/10918067/diff/1/src/snapshot-common.cc File src/snapshot-common.cc (right): http://codereview.chromium.org/10918067/diff/1/src/snapshot-common.cc#newcode92 src/snapshot-common.cc:92: bool success = V8::Initialize(&deserializer); Is there any plans to include a SnapshotByteSink that writes to file in mksnapshot so that it can be read here? http://codereview.chromium.org/10918067/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
