[+v8-dev] On Thu, Sep 2, 2010 at 15:17, <[email protected]> wrote:
> > http://codereview.chromium.org/3329006/diff/13003/17001 > > File include/v8.h (right): > > http://codereview.chromium.org/3329006/diff/13003/17001#newcode2363 > include/v8.h:2363: enum ObjectSpace { > Actually thinking about this again we could get rid of the > AllocationType enum, and have kObjectSpaceCode here instead. Allocation > in code space will always be executable and allocation in all other > spaces will be non-executable. > > This should have values 1:1 to v8::internal::AllocationSpace > > And then have some masks e.g. kObjectSpaceTenured = > kObjectSpaceTenuredPointerSpace | kObjectSpaceTenuredDataSpace | > kObjectSpaceMapSpace | kObjectSpaceCellSpace > > As far as I can see MemoryAllocator::AllocateRawMemory is only called > from a few places so tracking the space should not be that difficult. > > http://codereview.chromium.org/3329006/diff/13003/17001#newcode2384 > include/v8.h:2384: AllocationAction action); > This typedef is missing an int argument for the size of the allocation. > > http://codereview.chromium.org/3329006/diff/13003/17003 > > File src/spaces.cc (right): > > http://codereview.chromium.org/3329006/diff/13003/17003#newcode376 > src/spaces.cc:376: AllocationType type = (executable == EXECUTABLE) ? > kAllocationTypeCode : > Please format like this > > AllocationType type = > (executable == EXECUTABLE) ? kAllocationTypeCode : > kAllocationTypeData; > > http://codereview.chromium.org/3329006/diff/13003/17003#newcode380 > src/spaces.cc:380: filter.callback(filter.space, type, > kAllocationActionAllocate); > I think you should pass 0 here instead of filter.space whilt this is not > yet implemented. > > http://codereview.chromium.org/3329006/diff/13003/17003#newcode403 > src/spaces.cc:403: AllocationType type = (executable == EXECUTABLE) ? > kAllocationTypeCode : > Ditto. > > http://codereview.chromium.org/3329006/diff/13003/17004 > File src/spaces.h (right): > > http://codereview.chromium.org/3329006/diff/13003/17004#newcode657 > src/spaces.h:657: struct MemoryAllocationCallbackFilter { > How about renaming MemoryAllocationCallbackFilter to > MemoryAllocationCallbackRegistration? > > http://codereview.chromium.org/3329006/diff/13003/17004#newcode662 > src/spaces.h:662: : callback(callback), > 4 space indent of constructor initializer list, see > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constructor_Initializer_Lists#Constructor_Initializer_Lists > > You can fit the initializer list on one line then. > > http://codereview.chromium.org/3329006/diff/13003/17004#newcode667 > src/spaces.h:667: bool operator==(const MemoryAllocationCallbackFilter& > filter) const { > I don't like this == operator which only compares the callback. I > suggest replacing List<>::Contains with a loop instead. > > > http://codereview.chromium.org/3329006/show > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
