I'm not very keen on this one. The idea of having an array with constants
to
define what the various offsets mean seems like a second best solution
compared
with just having a set of different classes and virtual functions. Instead
of
the array you would have objects of the correct instance.
http://codereview.chromium.org/3467010/diff/1/3
File src/runtime.cc (right):
http://codereview.chromium.org/3467010/diff/1/3#newcode4743
src/runtime.cc:4743: index = search.Search(subject, index);
this is probably not getting inlined...
http://codereview.chromium.org/3467010/diff/1/4
File src/string-search.h (right):
http://codereview.chromium.org/3467010/diff/1/4#newcode69
src/string-search.h:69: static const int kStrategyOffset = 0;
This is nasty. Instead of an integer that indicates the strategy, how
about a pointer to an object that implements the strategy?
http://codereview.chromium.org/3467010/diff/1/4#newcode91
src/string-search.h:91: static inline bool NonAsciiString(Vector<const
char>) {
It is preferable for predicate functions to be positively named so you
don't get if (!Not....
http://codereview.chromium.org/3467010/diff/1/4#newcode105
src/string-search.h:105: // for later use.
The comment should reflect the fact that that is currently always the
case.
http://codereview.chromium.org/3467010/diff/1/4#newcode152
src/string-search.h:152: // UC16 Needle.
Instead of this comment it is better to have an ASSERT on the sizeof.
http://codereview.chromium.org/3467010/diff/1/4#newcode162
src/string-search.h:162: typedef int
(*SearchStrategy)(StringSearch<PatternChar, SubjectChar>*, // NOLINT -
it's not a cast!
The NOLINT is masking an overlong line here.
http://codereview.chromium.org/3467010/diff/1/4#newcode392
src/string-search.h:392: static int
InitialStrategy(StringSearch<PatternChar, SubjectChar>* search,
This is named like a sort of a getter, but it mutates the object.
Should it be called "InitializeStrategy"?
http://codereview.chromium.org/3467010/diff/1/4#newcode438
src/string-search.h:438: switch (id) {
This could be a lookup in a static const array.
http://codereview.chromium.org/3467010/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
