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)
On 2010/05/20 10:28:35, Søren Gjesse wrote:
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)
Done.
http://codereview.chromium.org/2069013/diff/1/2#newcode715
src/serialize.cc:715: case where + how + within + space_number:
On 2010/05/20 10:28:35, Søren Gjesse wrote:
How about adding
ASSERT((where & how & within & space_number) == 0);
here?
I added some asserts. The one you suggested wouldn't trigger unless
they all overlap, so I made it a little different.
http://codereview.chromium.org/2069013/diff/1/2#newcode762
src/serialize.cc:762: new_object = reinterpret_cast<Object*>(
\
On 2010/05/20 10:28:35, Søren Gjesse wrote:
This is not an object pointer any more.
I tried to make it into an Address, but that causes strict aliasing
issues and didn't really reduce the number of casts. Comment added
instead.
http://codereview.chromium.org/2069013/diff/1/2#newcode807
src/serialize.cc:807: #define COMPACT_ONE_PER_SPACE(where, how, within)
\
On 2010/05/20 10:28:35, Søren Gjesse wrote:
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?
Renamed to ALL_SPACES and added comment.
http://codereview.chromium.org/2069013/diff/1/2#newcode825
src/serialize.cc:825:
On 2010/05/20 10:28:35, Søren Gjesse wrote:
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.
Added lots of comments.
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,
On 2010/05/20 10:28:35, Søren Gjesse wrote:
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.
Done.
http://codereview.chromium.org/2069013/diff/1/3#newcode201
src/serialize.h:201: static const int kPointerEncodingMask = 0x40;
On 2010/05/20 10:28:35, Søren Gjesse wrote:
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?
Done.
http://codereview.chromium.org/2069013/diff/1/3#newcode209
src/serialize.h:209: static const int kPointerOffsetMask = 0x80;
On 2010/05/20 10:28:35, Søren Gjesse wrote:
Ditto.
Done.
http://codereview.chromium.org/2069013/diff/1/3#newcode239
src/serialize.h:239:
On 2010/05/20 10:28:35, Søren Gjesse wrote:
Can we have static assets for kNumberOfSpaces == 9 and no overlap of
the
kPointedToMask, kPointerEncodingMask and kPointerOffsetMask?
I think the switch takes care of that. If they overlap then you get
multiply defined labels immediately.
http://codereview.chromium.org/2069013/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
