KMP string search using direct memory access and templates for type specialization.

[Lasse Reichstein, 2008-10-06 12:17:54]

A little code review.
I am aware of the six formatting-errors in runtime.cc.

[Lasse Reichstein, 2008-10-06 12:18:34]

A small code review.
I know about the formatting-errors in runtime.cc.

[Lasse Reichstein, 2008-10-06 12:19:08]

On 2008/10/06 12:17:54, lrn wrote:
> A little code review.
> I am aware of the six formatting-errors in runtime.cc.

(ignore, typo in reviewer e-mail address)

[Christian Plesner Hansen, 2008-10-06 12:40:26]

Lgtm

[Christian Plesner Hansen, 2008-10-06 12:47:19]

...and here are my comments.

http://codereview.chromium.org/6269/diff/1/5
File src/runtime.cc (right):

http://codereview.chromium.org/6269/diff/1/5#newcode935
Line 935: for(;;) {
Flat strings are relatively simple in structure so it should be possible to get
a hold of the string contents with straight-line code.

http://codereview.chromium.org/6269/diff/1/5#newcode1006
Line 1006: Vector<const schar> string,
This argument should be on the same line as the function header with the
remaining arguments below it, indented to the same level.

http://codereview.chromium.org/6269/diff/1/5#newcode1009
Line 1009: for (int i = start_index, n = string.length(); i < n; i++) {
I would move the declaration of n out before the loop.

http://codereview.chromium.org/6269/diff/1/5#newcode1028
Line 1028: for (int i = start_index, n = subject_length - pattern_length; i <=
n; i++) {
I would move the declaration of n out just before the loop.

http://codereview.chromium.org/6269/diff/1/5#newcode1060
Line 1060: int length = pattern.length();
Isn't this identical to pattern_length?

[Lasse Reichstein, 2008-10-07 09:16:28]

http://codereview.chromium.org/6269/diff/1/5
File src/runtime.cc (right):

http://codereview.chromium.org/6269/diff/1/5#newcode935
Line 935: for(;;) {
On 2008/10/06 12:47:19, Christian Plesner Hansen wrote:
> Flat strings are relatively simple in structure so it should be possible to
get
> a hold of the string contents with straight-line code.

Done.

http://codereview.chromium.org/6269/diff/1/5#newcode1006
Line 1006: Vector<const schar> string,
On 2008/10/06 12:47:19, Christian Plesner Hansen wrote:
> This argument should be on the same line as the function header with the
> remaining arguments below it, indented to the same level.

Done.

http://codereview.chromium.org/6269/diff/1/5#newcode1009
Line 1009: for (int i = start_index, n = string.length(); i < n; i++) {
On 2008/10/06 12:47:19, Christian Plesner Hansen wrote:
> I would move the declaration of n out before the loop.

I consider
 for (int i = ..., n = ...; i<n; i++)
an idiom, and prefer to limit the scope of n to where it's needed.

http://codereview.chromium.org/6269/diff/1/5#newcode1060
Line 1060: int length = pattern.length();
On 2008/10/06 12:47:19, Christian Plesner Hansen wrote:
> Isn't this identical to pattern_length?

Indeed, it should be removed. Done.

http://codereview.chromium.org/6269/diff/1/5#newcode1122
Line 1122: sub->TryFlatten();
Doesn't handle failure to flatten.
Changed to the handle based FlattenString.