Erik, I addressed your comments.
If there be no new ones I will land this CL tomorrow as we agreed. http://codereview.chromium.org/5987005/diff/1/src/spaces.cc File src/spaces.cc (right): http://codereview.chromium.org/5987005/diff/1/src/spaces.cc#newcode326 src/spaces.cc:326: void* MemoryAllocator::ReserveAlignedMemory(const size_t requested, On 2010/12/22 13:48:27, Erik Corry wrote:
Shouldn't this return an Address?
Done. I eradicated void* usages. http://codereview.chromium.org/5987005/diff/1/src/spaces.cc#newcode862 src/spaces.cc:862: ASSERT(IsAddressAligned(chunk_base_, 2*maximum_semispace_capacity, 0)); On 2010/12/22 13:48:27, Erik Corry wrote:
spaces around '*'
Done. http://codereview.chromium.org/5987005/diff/1/src/spaces.h File src/spaces.h (right): http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode116 src/spaces.h:116: // MemoryChunk represents memory region owned by a specific space. On 2010/12/22 13:48:27, Erik Corry wrote:
represents memory -> represents a memory
Done. http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode130 src/spaces.h:130: MemoryChunk* next_chunk() const { return next_; } On 2010/12/22 13:48:27, Erik Corry wrote:
For getters the function is normally called the same as the variable,
so the
variable should be called next_chunk_ or the getter should be called
next() Done. http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode154 src/spaces.h:154: return flags_ & (1 << flag); On 2010/12/22 13:48:27, Erik Corry wrote:
Implicit conversions from int to bool are not allowed, so you need to
add "!= 0"
here.
Done. http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode234 src/spaces.h:234: return static_cast<Page*>(next_chunk()); On 2010/12/22 13:48:27, Erik Corry wrote:
Should we assert that the next chunk is a page?
I added an assert to check that pages have the same owner. There is currently no page type flag stored in the page itself. Maybe we should consider eradicating LargePage type completely. http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode270 src/spaces.h:270: // only after PagedSpace::PrepareForMarkCompact was called. On 2010/12/22 13:48:27, Erik Corry wrote:
Should the comment go away?
Done. http://codereview.chromium.org/5987005/diff/1/src/spaces.h#newcode658 src/spaces.h:658: static size_t capacity_; On 2010/12/22 13:48:27, Erik Corry wrote:
I don't like unsigned types much, so I tend to prefer intptr_t to
size-t I like signed types :-) But the reason I changed these declaration is different: interface to MemoryAllocator (and platform interface) was initially based on size_t so casts inside were a bit weird. http://codereview.chromium.org/5987005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
