Make Win64 compile.

Make Win64 compile.

[Lasse Reichstein, 2011-07-15 13:13:42]

Makes the GC branch compile on Win64.
It doesn't necessarily work.
This was fixed by adding static_cast<int>'s - if anyone can see a case where
this might fail, do fix the original code to not do implicit casts from
intptr_t, size_t or ptrdiff_t to int.
And please stop adding them.

[Erik Corry, 2011-07-15 21:37:32]

LGTM

Did you consider making Free(...) take an intptr_t for the size?

http://codereview.chromium.org/7389008/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7389008/diff/1/src/heap.cc#newcode4995
src/heap.cc:4995: if (!ConfigureHeapDefault()) {
This looks like an unnecessary change.

http://codereview.chromium.org/7389008/diff/1/src/heap.cc#newcode5055
src/heap.cc:5055: if (code_space_ == NULL || !code_space_->Setup()) {
These all fit on one line.

http://codereview.chromium.org/7389008/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

http://codereview.chromium.org/7389008/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:435: intptr_t current = marking_deque_.bottom();
I think making all these int instead of intptr_t should obviate the need for the
static cast below.

http://codereview.chromium.org/7389008/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/7389008/diff/1/src/mark-compact.cc#newcode3009
src/mark-compact.cc:3009: int
MarkCompactCollector::SweepConservatively(PagedSpace* space, Page* p) {
Perhas this should return intptr_t?

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode323
src/spaces.cc:323: }
Unneeded change.

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode326
src/spaces.cc:326: static_cast<int>(OS::AllocateAlignment()));
RoundUp takes a size_t and an int?  Perhaps that's the bug.

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode671
src/spaces.cc:671: allocation_info_.top =
reinterpret_cast<Address>(static_cast<uintptr_t>(2));
2?

Don't understand this at all, but it seems the number should have a name, in
which case it can have the right type.

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode865
src/spaces.cc:865: if (base == NULL) {
Grrr.

http://codereview.chromium.org/7389008/diff/1/src/store-buffer.h
File src/store-buffer.h (right):

http://codereview.chromium.org/7389008/diff/1/src/store-buffer.h#newcode87
src/store-buffer.h:87: static const int kOldStoreBufferLength =
kStoreBufferLength * 16;
Merge artifact?

http://codereview.chromium.org/7389008/diff/1/src/utils.h
File src/utils.h (right):

http://codereview.chromium.org/7389008/diff/1/src/utils.h#newcode123
src/utils.h:123: static inline T RoundUp(T x, intptr_t m) {
This would seem to make unneccessary some of the other changes.

[Lasse Reichstein, 2011-08-01 12:40:32]

http://codereview.chromium.org/7389008/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7389008/diff/1/src/heap.cc#newcode4995
src/heap.cc:4995: if (!ConfigureHeapDefault()) {
Added to allow a breakpoint at the return.
It's a simple change, so anyone needing it can reintroduce it, so I'll can
revert it again.
In fact, I'll revert the entire file.

http://codereview.chromium.org/7389008/diff/1/src/heap.cc#newcode5109
src/heap.cc:5109: CHECK_EQ(isolate_, isolate());
This should probably have been ASSERT_EQ.

http://codereview.chromium.org/7389008/diff/1/src/incremental-marking.cc
File src/incremental-marking.cc (right):

http://codereview.chromium.org/7389008/diff/1/src/incremental-marking.cc#newc...
src/incremental-marking.cc:435: intptr_t current = marking_deque_.bottom();
True, there is no need for intptr_t-ness here.

http://codereview.chromium.org/7389008/diff/1/src/mark-compact.cc
File src/mark-compact.cc (right):

http://codereview.chromium.org/7389008/diff/1/src/mark-compact.cc#newcode3009
src/mark-compact.cc:3009: int
MarkCompactCollector::SweepConservatively(PagedSpace* space, Page* p) {
Should probably be size_t, but intptr_t will do (that's the type that the value
is later added to).

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc
File src/spaces.cc (right):

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode323
src/spaces.cc:323: }
I added it in order to be able to make a breakpoint at the return. Will revert
them all.

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode326
src/spaces.cc:326: static_cast<int>(OS::AllocateAlignment()));
It's taking intptr_t for its second argument actually. That's what it's using
internally when rounding. I'll fix the cast.

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode334
src/spaces.cc:334: return NULL;
reverted.

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode671
src/spaces.cc:671: allocation_info_.top =
reinterpret_cast<Address>(static_cast<uintptr_t>(2));
Sorry, leftover from debugging. Should be NULL again.

http://codereview.chromium.org/7389008/diff/1/src/spaces.cc#newcode865
src/spaces.cc:865: if (base == NULL) {
Curly-brace-ophobia is treatable.

http://codereview.chromium.org/7389008/diff/1/src/store-buffer.h
File src/store-buffer.h (right):

http://codereview.chromium.org/7389008/diff/1/src/store-buffer.h#newcode87
src/store-buffer.h:87: static const int kOldStoreBufferLength =
kStoreBufferLength * 16;
No, out of memory error with larger value.

http://codereview.chromium.org/7389008/diff/1/src/utils.h
File src/utils.h (right):

http://codereview.chromium.org/7389008/diff/1/src/utils.h#newcode123
src/utils.h:123: static inline T RoundUp(T x, intptr_t m) {
I have fixed one - hope it's all.

[Lasse Reichstein, 2011-08-01 12:47:05]

On 2011/07/15 21:37:32, Erik Corry wrote:
> LGTM
> 
> Did you consider making Free(...) take an intptr_t for the size?

I did, but that would require changing FreeList etc. Good idea (we should
probably never use ints for anything but known bounded loop variables), but this
patch is just supposed to make things *compile*. They don't work yet.