LGTM with a bunch of nits. Does this lint?
http://codereview.chromium.org/9038/diff/1/11 File src/compiler.cc (right): http://codereview.chromium.org/9038/diff/1/11#newcode160 Line 160: StringShape source_shape(*source); Pull out source_length in a variable here as you do below? http://codereview.chromium.org/9038/diff/1/9 File src/conversions.cc (right): http://codereview.chromium.org/9038/diff/1/9#newcode126 Line 126: int len = strlen(other); Introduced other_len and str_len? http://codereview.chromium.org/9038/diff/1/23 File src/factory.cc (right): http://codereview.chromium.org/9038/diff/1/23#newcode101 Line 101: second_shape), String); Put String on a new line aligned with Heap? http://codereview.chromium.org/9038/diff/1/6 File src/heap.cc (right): http://codereview.chromium.org/9038/diff/1/6#newcode1349 Line 1349: Object* Heap::AllocateConsString(String* first, StringShape first_shape, String* second, StringShape second_shape) { Isn't this line too long? http://codereview.chromium.org/9038/diff/1/6#newcode1454 Line 1454: buffer_shape = buffer; Please make the conversion explicit here. http://codereview.chromium.org/9038/diff/1/17 File src/objects-inl.h (right): http://codereview.chromium.org/9038/diff/1/17#newcode137 Line 137: bool Object::IsSeqString() { Is it too expensive to use StringShapes for these? This is pretty hard to read and it also seems error prone. http://codereview.chromium.org/9038/diff/1/8 File src/objects.cc (right): http://codereview.chromium.org/9038/diff/1/8#newcode2915 Line 2915: return true; Doesn't it make sense to check that the representation tag makes sense? http://codereview.chromium.org/9038/diff/1/13 File src/objects.h (right): http://codereview.chromium.org/9038/diff/1/13#newcode3026 Line 3026: // string can potentially alter its shape. Would it make sense to put in some debug code that adds a valid_ member to the StringShape class and sets it to false when flattened? Then each of the operations can assert that the shape is valid which could potentially catch nasty bugs. http://codereview.chromium.org/9038/diff/1/13#newcode3033 Line 3033: class StringShape { Are these intended to be only stack allocated? In that case, you should use BASE_EMBEDDED: class StringShape BASE_EMBEDDED { http://codereview.chromium.org/9038/diff/1/13#newcode3035 Line 3035: inline StringShape(String* s); One-argument constructors should be explicit. http://codereview.chromium.org/9038/diff/1/12 File src/runtime.cc (right): http://codereview.chromium.org/9038/diff/1/12#newcode2158 Line 2158: StringShape sshape(source); How about source_shape? http://codereview.chromium.org/9038/diff/1/12#newcode2187 Line 2187: StringShape dshape(destination); dest_shape or even destination_shape? http://codereview.chromium.org/9038/diff/1/12#newcode2271 Line 2271: StringShape sshape(source); source_shape? http://codereview.chromium.org/9038/diff/1/12#newcode2293 Line 2293: StringShape dshape(destination); destination_shape? http://codereview.chromium.org/9038 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
