LGTM. It would be good with a regression test case though -- even though it might be difficult to produce one -- and I would probably add a comment to the new block with AssertNoAllocation explaining the purpose.
On Sun, Mar 15, 2009 at 10:59 PM, <[email protected]> wrote: > > Reviewers: Lasse Reichstein, > > Description: > Fix GC related crash bug in search-replace. > > Please review this at http://codereview.chromium.org/42214 > > SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/ > > Affected files: > M src/runtime.cc > > > Index: src/runtime.cc > =================================================================== > --- src/runtime.cc (revision 1512) > +++ src/runtime.cc (working copy) > @@ -1146,10 +1146,10 @@ > StringBuilderSubstringPosition::is_valid(from)) { > int encoded_slice = StringBuilderSubstringLength::encode(length) | > StringBuilderSubstringPosition::encode(from); > - AddElement(Smi::FromInt(encoded_slice)); > + AddElement(Handle<Object>(Smi::FromInt(encoded_slice))); > } else { > Handle<String> slice = Factory::NewStringSlice(subject_, from, to); > - AddElement(*slice); > + AddElement(slice); > } > IncrementCharacterCount(length); > } > @@ -1160,7 +1160,7 @@ > StringShape shape(*string); > int length = string->length(shape); > if (length > 0) { > - AddElement(*string); > + AddElement(string); > if (!shape.IsAsciiRepresentation()) { > is_ascii_ = false; > } > @@ -1220,7 +1220,7 @@ > } > > > - void AddElement(Object* element) { > + void AddElement(Handle<Object> element) { > ASSERT(element->IsSmi() || element->IsString()); > // Extend parts_ array if necessary. > if (parts_->length() == part_count_) { > @@ -1229,7 +1229,7 @@ > parts_->CopyTo(0, *extended_array, 0, part_count_); > parts_ = extended_array; > } > - parts_->set(part_count_, element); > + parts_->set(part_count_, *element); > part_count_++; > } > > @@ -1551,12 +1551,16 @@ > > do { > ASSERT(last_match_info_handle->HasFastElements()); > - FixedArray* match_info_array = last_match_info_handle->elements(); > + int start, end; > + { > + AssertNoAllocation a; > + FixedArray* match_info_array = last_match_info_handle->elements(); > > - ASSERT_EQ(capture_count * 2 + 2, > - RegExpImpl::GetLastCaptureCount(match_info_array)); > - int start = RegExpImpl::GetCapture(match_info_array, 0); > - int end = RegExpImpl::GetCapture(match_info_array, 1); > + ASSERT_EQ(capture_count * 2 + 2, > + RegExpImpl::GetLastCaptureCount(match_info_array)); > + start = RegExpImpl::GetCapture(match_info_array, 0); > + end = RegExpImpl::GetCapture(match_info_array, 1); > + } > > if (prev < start) { > builder.AddSubjectSlice(prev, start); > > > > > > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
