LGTM.
http://codereview.chromium.org/5100002/diff/7001/src/handles.cc File src/handles.cc (right): http://codereview.chromium.org/5100002/diff/7001/src/handles.cc#newcode510 src/handles.cc:510: bool with_last_line) { I wonder whether this function is performance critical or not. http://codereview.chromium.org/5100002/diff/7001/src/handles.cc#newcode518 src/handles.cc:518: position = search.Search(src, position); Not calling Runtime::StringMatch is good (it's an "externally visible" function, so it shouldn't be reused). However, if the function is performance critical, this seems overkill for a single-character string. It will always do the "SingleCharSearch", which boils down to something like: int index; if (sizeof(SourceChar) == 1) { char* pos = reinterpret_cast<char*>( memchr(src.start() + position, '\n', src.length())); index = pos ? static_cast<, n = , n = int>(pos - src.start()) : -1; } else { ASSERT(sizeof(SourceChar) == 2); int n = src.length(); for (index = position; index < n; index++) { if (src[i] == '\n') { break; } } if (index == n) index = -1; } (not tested!) I'd rather inline thee code, or extract it from SingleCharSearch to a static function/template that can be called from outside the string-search object as well. http://codereview.chromium.org/5100002/diff/7001/src/handles.cc#newcode528 src/handles.cc:528: // Pass 2: Fill in line ends positions On the other hand, if the function isn't performance critical, then doing two iterations to first find the count and then collect the values seems overly complex, compared to just collecting the values in a List or Collector during the first iteration. Performance-wise it also seems like doing double the work in iteration instead double the work copying the result afterwards. It does save (C-heap) memory, though. http://codereview.chromium.org/5100002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
