2009/9/29 Anton Muhin <[email protected]> > Erik, thanks a lot for review, I'll respond in details somewhat later > (in an hour or two). Just some minor observations. Regarding more > generic approach: I thought of treading buffer to fill into > string-specific write methods which would allow us to get rid of > flattening completely. Probably would investigate it. And one thing > I forgot in this version: I should shortcut copying after > flattening---if string gets flat, it'd be possible to copy it with one > move. Will update a CL when responding to your comments. >
You could use FlattenString from handles.h. Then you can be sure that it is flat afterwards. > > yours, > anton. > > On Tue, Sep 29, 2009 at 12:40 PM, <[email protected]> wrote: > > LGTM. I can't help thinking we could make this more general, but so far > I > > haven't come up with a nice scheme myself. > > > > > > http://codereview.chromium.org/248031/diff/3001/3002 > > File src/api.cc (right): > > > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2448 > > Line 2448: // TODO(antonm): consider some fancier ways (like Duff's). > > Duff's Device is only useful for IO ports, not for real memory. > > > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2480 > > Line 2480: i::StringShape shape(*str); > > I have been meaning to get rid of StringShape because the performance > > benefits were very minimal while the risk of errors was rather large. > > If the benefit here is measurable I would encourage you to make a new > > scope for the shape so that it goes out of scope before the string > > flattening below. This will make it harder for someone to accidentally > > use it after a flattening or GC has made it invalid. > > > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2483 > > Line 2483: switch (shape.representation_tag()) { > > You should switch on the full representation tag here so you avoid an if > > in each case. > > > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2499 > > Line 2499: if (first_shape.representation_tag() == i::kSeqStringTag || > > Consider making this a switch too. > > > > http://codereview.chromium.org/248031/diff/3001/3002#newcode2508 > > Line 2508: if (second->length() == 0) { > > How about 'int second_length = second->length()'. That saves you > > looking it up more than once. Then you can do > > if (second_length <= 1) { > > bool only_from_first = (end < length - second_length) > > and you can combine the two branches. > > > > http://codereview.chromium.org/248031 > > > -- Erik Corry, Software Engineer Google Denmark ApS. CVR nr. 28 86 69 84 c/o Philip & Partners, 7 Vognmagergade, P.O. Box 2227, DK-1018 Copenhagen K, Denmark. --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
