LGTM with some comments and suggestions.
https://codereview.chromium.org/11638037/diff/11001/src/objects-inl.h File src/objects-inl.h (right): https://codereview.chromium.org/11638037/diff/11001/src/objects-inl.h#newcode2859 src/objects-inl.h:2859: : op_(op) { Please initialize is_one_byte_, some compilers whine if it's not. https://codereview.chromium.org/11638037/diff/11001/src/objects-inl.h#newcode2866 src/objects-inl.h:2866: ConsStringIteratorOp* op) Wouldn't it be cleaner to put offset as last and optional argument? https://codereview.chromium.org/11638037/diff/11001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/11638037/diff/11001/src/objects.cc#newcode1542 src/objects.cc:1542: String* string) { no line break necessary anymore. https://codereview.chromium.org/11638037/diff/11001/src/objects.cc#newcode7501 src/objects.cc:7501: class RawStringComparator { You could derive this class from AllStatic. https://codereview.chromium.org/11638037/diff/11001/src/objects.cc#newcode7534 src/objects.cc:7534: class State { Since this is another visitor, I was wondering whether it would be better (e.g. smaller code size) to use virtual methods and abstract classes for Visitor and ConsOp so that we don't have to templatize String::Visit for all combinations. Not sure if performance would visibly suffer due to vtable lookup... https://codereview.chromium.org/11638037/diff/11001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/11638037/diff/11001/src/objects.h#newcode7902 src/objects.h:7902: inline void Reset(String* string, unsigned offset); Save a line using optional argument with default value :) https://codereview.chromium.org/11638037/diff/11001/src/runtime.h File src/runtime.h (right): https://codereview.chromium.org/11638037/diff/11001/src/runtime.h#newcode595 src/runtime.h:595: ConsStringIteratorOp* string_locale_compare_it1() { Do we really need a second pair of ConsStringIteratorOp? Can't we reuse the first pair where the second pair is used? https://codereview.chromium.org/11638037/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
