On 2010/07/06 06:55:46, Lasse Reichstein wrote:
LGTM, with comments.
http://codereview.chromium.org/2868046/diff/1/2
File src/runtime.cc (right):
http://codereview.chromium.org/2868046/diff/1/2#newcode2288
src/runtime.cc:2288: static Object*
StringReplaceRegExpWithEmptyString(InputSeqString* subject,
Just change InputSeqString to String and drop the parameterization.
Sequential and External strings don't share any features not also in
String
anyway.
Right, I too realized we don't really depend on the input type.
http://codereview.chromium.org/2868046/diff/1/2#newcode2291
src/runtime.cc:2291: ASSERT(subject->IsFlat());
but add
ASSERT(!subject->IsConsString());
As discussed by IM this is not required.
http://codereview.chromium.org/2868046/diff/1/2#newcode2446
src/runtime.cc:2446: subject = ConsString::cast(subject)->first();
Aw heck, how could I forget so soon after I fixed the other RegExp
functions.
http://codereview.chromium.org/2868046/diff/1/2#newcode2448
src/runtime.cc:2448: if (subject->IsAsciiRepresentation()) {
The above change would avoid avoid casting the string completely.
Sure, now the code is simpler.
I addressed the TODO by adding kHasAsciiEncoding constant to some strings
types.
It can be statically checked by templated code.
I also added a simple test.
Please take another look.
Thanks,
Vitaly
http://codereview.chromium.org/2868046/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
