first round of comments:
http://codereview.chromium.org/7977001/diff/5001/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7977001/diff/5001/src/heap.cc#newcode2535 src/heap.cc:2535: Add a note about the layout of the fixed array, e.g., that we keep the number of current elements in the first entry and then the actual strings in the subsequent entries. http://codereview.chromium.org/7977001/diff/5001/src/heap.cc#newcode2546 src/heap.cc:2546: if (allocation->IsFailure()) return allocation; Why not just use our standard way of doing this using an ToObject on allocation and then casting the result afterwards. ToObjectUnchecked just seems wrong here http://codereview.chromium.org/7977001/diff/5001/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/7977001/diff/5001/src/objects-inl.h#newcode4112 src/objects-inl.h:4112: ASSERT(is_array_index_); I think the name of this function is not descriptive anymore when this is the case? Additionally, why no just have this method and call AddCharacterNoIndex if is_array_index_ is false? This will make the code using this easier to read http://codereview.chromium.org/7977001/diff/5001/src/objects.h File src/objects.h (right): http://codereview.chromium.org/7977001/diff/5001/src/objects.h#newcode6171 src/objects.h:6171: bool IsSeqAsciiString() { Why this, we can always just call IsSequentialAscii right? Is this just to be able to assert this? http://codereview.chromium.org/7977001/diff/5001/src/objects.h#newcode6228 src/objects.h:6228: ASSERT(this->Object::IsSeqTwoByteString()); Same question as for ascii http://codereview.chromium.org/7977001/diff/5001/src/runtime.cc File src/runtime.cc (right): http://codereview.chromium.org/7977001/diff/5001/src/runtime.cc#newcode9054 src/runtime.cc:9054: source = FlattenGetString(source); As discussed offline, please change comment in handles.h which states that this will return a failure object on OOM http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc File test/cctest/test-api.cc (right): http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc#newcode15348 test/cctest/test-api.cc:15348: } Could we do this again with some GC's mixed in? http://codereview.chromium.org/7977001/diff/5001/test/cctest/test-api.cc#newcode15349 test/cctest/test-api.cc:15349: Also, even though the current code just removes the first instance it fines I think it would be nice to validate that we can add the same string several times http://codereview.chromium.org/7977001/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
