Move string-search functions to separate file.

Move string-search functions to separate file.

Committed: http://code.google.com/p/v8/source/detail?r=5440

[sandholm, 2010-09-09 08:55:39]

LGTM

Did you consider moving (the Vector variant of) SearchStringMultiple as well?

On 2010/09/09 08:34:04, Lasse Reichstein wrote:
>

[Kevin Millikin (Chromium), 2010-09-09 09:10:50]

Drive by.

http://codereview.chromium.org/3291021/diff/1/3
File src/string-search.h (right):

http://codereview.chromium.org/3291021/diff/1/3#newcode85
src/string-search.h:85: static int bad_char_occurrence[kBMAlphabetSize];
This does not belong in a header file.

http://codereview.chromium.org/3291021/diff/1/3#newcode101
src/string-search.h:101: static void BoyerMoorePopulateBadCharTable(Vector<const
pchar> pattern) {
This must be declared inline if it's in a header file.

I don't think you want to inline it anyway.

[Kevin Millikin (Chromium), 2010-09-09 09:31:31]

I have a few more comments, which are basically that you should (briefly)
"modernize" this code while moving it.

http://codereview.chromium.org/3291021/diff/1/3
File src/string-search.h (right):

http://codereview.chromium.org/3291021/diff/1/3#newcode1
src/string-search.h:1: // Copyright 2006-2010 the V8 project authors. All rights
reserved.
Just 2010.

http://codereview.chromium.org/3291021/diff/1/3#newcode39
src/string-search.h:39: static const int kBMMaxShift = 0xff;
I have no idea why this is a hex constant.

http://codereview.chromium.org/3291021/diff/1/3#newcode47
src/string-search.h:47: static const int kBMAlphabetSize = 0x100;
Nor this.

http://codereview.chromium.org/3291021/diff/1/3#newcode58
src/string-search.h:58: inline void init(int needle_length) {
No reason for this to be inline, is there?  Naming convention would suggest
Init.

http://codereview.chromium.org/3291021/diff/1/3#newcode68
src/string-search.h:68: inline int& suffix(int index) {
Consider a more v8-ey pair of SuffixAt and SetSuffixAt getter and setter
functions, rather than returning a reference.

http://codereview.chromium.org/3291021/diff/1/3#newcode100
src/string-search.h:100: template <typename pchar>
Typenames template parameters are normally capitalized.

http://codereview.chromium.org/3291021/diff/1/3#newcode103
src/string-search.h:103: int start = pattern.length() < kBMMaxShift ? 0
Maybe Max(0, pattern.length() - kBMMaxShift)?

Otherwise we normally format this as

int start = pattern.length() < kBMMaxSHift
    ? 0
    : pattern.length() - kBMMaxSHift;

http://codereview.chromium.org/3291021/diff/1/3#newcode137
src/string-search.h:137: for (int i = m; i > start;) {
Since this is not really a counted loop, it's probably clearer written as a
while loop.

http://codereview.chromium.org/3291021/diff/1/3#newcode138
src/string-search.h:138: for (pchar c = pattern[i - 1]; suffix <= m && c !=
pattern[suffix - 1];) {
Why not put the update expression in the for loop, or rewrite as a while loop.

This way the reader has to hunt for the update and then wonder if it's doing
something clever (I guess it's not).

http://codereview.chromium.org/3291021/diff/1/3#newcode144
src/string-search.h:144: i--;
Why go out of your way to avoid

bmgs_buffers.suffix(--i) = --suffix;

Et cetera.

[Lasse Reichstein, 2010-09-09 09:51:00]

http://codereview.chromium.org/3291021/diff/1/3
File src/string-search.h (right):

http://codereview.chromium.org/3291021/diff/1/3#newcode85
src/string-search.h:85: static int bad_char_occurrence[kBMAlphabetSize];
True. I was moving this code to a separate file in anticipation of further
cleanup, but I'll move these two arrays immediately (and the start working on
naming etc, suggestions are welcome).

http://codereview.chromium.org/3291021/diff/1/3#newcode101
src/string-search.h:101: static void BoyerMoorePopulateBadCharTable(Vector<const
pchar> pattern) {
It has to be an a header file because it's a template function.