Thanks for this patch! I have some drive-by comments.
https://codereview.chromium.org/11299163/diff/1/src/objects.h File src/objects.h (right): https://codereview.chromium.org/11299163/diff/1/src/objects.h#newcode7195 src/objects.h:7195: if (state_ == TWO_BYTE) return buffer_.length() / 2; you could do an if (state_ == ASCII) { ... } else { ASSERT(state == TWO_BYTE) { ... } to be cleaner. Even cooler would be without any branching, like, ASSERT(ASCII == 0 && TWO_BYTE == 1); ASSERT(state_ == ASCII || state_ == TWO_BYTE); return buffer_.length() >> state_; (Make sure the enum elements have values explicitly assigned.) https://codereview.chromium.org/11299163/diff/1/src/runtime.cc File src/runtime.cc (right): https://codereview.chromium.org/11299163/diff/1/src/runtime.cc#newcode2860 src/runtime.cc:2860: zone); Since we are doing the IsAscii() check (implied in Length()) in any case, couldn't we leave it with if (pattern_content.IsAscii()) { Vector<const char> pattern_vector = pattern_content.ToAsciiVector(); if (pattern_vector.length() == 1) { ... } else { ... } } else { Vector<const uc16> pattern_vector = pattern_content.ToUC16Vector(); if (pattern_vector.length() == 1) { ... } else { ... } } I know this is a bit more verbose, but this way we don't have to introduce Length() and also avoid the redundant ascii check. https://codereview.chromium.org/11299163/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
