On 2012/11/26 08:45:30, Yang wrote:
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.
LGTM. I'll just land this CL with my suggested changes. :)
https://codereview.chromium.org/11299163/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
