Fix crash: handle all flat string types in regexp replace.

Fix crash: handle all flat string types in regexp replace.

Committed: http://code.google.com/p/v8/source/detail?r=5025

[Lasse Reichstein, 2010-07-06 06:55:46]

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.

http://codereview.chromium.org/2868046/diff/1/2#newcode2291
src/runtime.cc:2291: ASSERT(subject->IsFlat());
but add 
 ASSERT(!subject->IsConsString());

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.

[Vitaly Repeshko, 2010-07-06 10:42:21]

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

[Lasse Reichstein, 2010-07-06 12:09:36]

LGTM

http://codereview.chromium.org/2868046/diff/5001/6002
File src/runtime.cc (right):

http://codereview.chromium.org/2868046/diff/5001/6002#newcode2443
src/runtime.cc:2443: if (subject->IsAsciiRepresentation() ||
subject->HasOnlyAsciiChars()) {
Does subject->IsAsciiRepresentation() imply subject->HasOnlyAsciiChars()? (I.e.,
could we do with just HasOnlyAsciiChars?)

[Vitaly Repeshko, 2010-07-06 12:22:39]

Thanks! Submitted.


-- Vitaly

http://codereview.chromium.org/2868046/diff/5001/6002
File src/runtime.cc (right):

http://codereview.chromium.org/2868046/diff/5001/6002#newcode2443
src/runtime.cc:2443: if (subject->IsAsciiRepresentation() ||
subject->HasOnlyAsciiChars()) {
On 2010/07/06 12:09:36, Lasse Reichstein wrote:
> Does subject->IsAsciiRepresentation() imply subject->HasOnlyAsciiChars()?
(I.e.,
> could we do with just HasOnlyAsciiChars?)

Done.