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