http://codereview.chromium.org/243051/diff/2001/3001 File src/heap.cc (right):
http://codereview.chromium.org/243051/diff/2001/3001#newcode1695 Line 1695: Object* result = Allocate(map, pretenure ? LO_SPACE : NEW_SPACE); On 2009/10/01 12:11:15, Kevin Millikin wrote: > 1. You should not rely on NOT_PRETENURED being false and PRETENURED being true. > Elsewhere in the file you will see: > AllocationSpace space = (pretenure == TENURED) ? ... > Object* result = Allocate(map, space); > 2. A pretenured cons string should not go in LO_SPACE (the large object space > for objects bigger than ~8K). It should go in OLD_POINTER_SPACE. Done. http://codereview.chromium.org/243051/diff/2001/3001#newcode1697 Line 1697: ASSERT(pretenure ? !InNewSpace(result) : InNewSpace(result)); On 2009/10/01 12:11:15, Kevin Millikin wrote: > Same comment about (not) relying on NOT_PRETENURED comparing as false and > PRETENURED comparing as true. Done. http://codereview.chromium.org/243051/diff/2001/3001#newcode1699 Line 1699: cons_string->set_first(first, SKIP_WRITE_BARRIER); On 2009/10/01 12:11:15, Kevin Millikin wrote: > If the cons string was allocated in the old generation, it is not safe to skip > the write barrier here. Done. http://codereview.chromium.org/243051/diff/2001/3002 File src/heap.h (right): http://codereview.chromium.org/243051/diff/2001/3002#newcode557 Line 557: static Object* AllocateConsString(String* first, String* second, On 2009/10/01 12:11:15, Kevin Millikin wrote: > The normal style is one parameter per line if they don't fit on the first line. > They should all be aligned: > static Object* AllocateConsString(String* first, > String* second, > PretenureFlag pretenure = NOT_TENURED); > or else > static Object* AllocateConsString( > String* first, > String* second, > PretenureFlag pretenure = NOT_TENURED); Done. http://codereview.chromium.org/243051/diff/2001/3003 File src/parser.cc (right): http://codereview.chromium.org/243051/diff/2001/3003#newcode2818 Line 2818: On 2009/10/01 12:11:15, Kevin Millikin wrote: > We do not allow lines that end in whitespace like this one. This should be > flagged by tools/presubmit.py. right now presubmit.py gives a crash for me. I'll try and get it working. Done. http://codereview.chromium.org/243051/diff/2001/3003#newcode2868 Line 2868: On 2009/10/01 12:11:15, Kevin Millikin wrote: > Another line that ends in whitespace (I don't think you need a blank line here > anyway). Done. http://codereview.chromium.org/243051/diff/2001/3003#newcode2869 Line 2869: Object* object = Heap::AllocateConsString(*str1, *str2, TENURED); On 2009/10/01 12:11:15, Kevin Millikin wrote: > As indicated in the comment in heap.h, AllocateConsString can return a failure > (not safe to stick in a handle) and will not force a GC. You should be calling > Factory::NewConsString. Done. http://codereview.chromium.org/243051 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
