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 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
