sorry for double post---500 error. yours, anton.
On Wed, Apr 7, 2010 at 3:37 PM, <[email protected]> wrote: > > http://codereview.chromium.org/1594017/diff/8001/9002 > File src/api.cc (right): > > http://codereview.chromium.org/1594017/diff/8001/9002#newcode2731 > src/api.cc:2731: EnsureInitialized("v8::String::Flatten()"); > I'd add ON_BAILOUT as well (or IsDeadCheck) as this operation has > side-effects, not a pure query. > > http://codereview.chromium.org/1594017/diff/8001/9003 > File test/cctest/test-strings.cc (right): > > http://codereview.chromium.org/1594017/diff/8001/9003#newcode357 > test/cctest/test-strings.cc:357: v8::Local<v8::String> cons = > v8::String::Concat(a,b); > I'd suspect that would create a flat string: v8::String::Concat > delegates to v8::internal::Factory::NewConsString which calls > Heap::AllocateConsString. > > This function > (https://cs.corp.google.com/p#chrome/trunk/src/v8/src/heap.cc&q=AllocateConsString&sa=N&cd=2&ct=rc) > would flatten result if its length is less than > String::kMinNonFlatLength which is currently 13 (see objects.h). > > So you should probably pick longer strings and maybe assert that result > string is cons. > > http://codereview.chromium.org/1594017/diff/8001/9003#newcode364 > test/cctest/test-strings.cc:364: for (i = 0; i < 3; i++) > by no mean insisting, but as you don't reuse i after first for-loop, I'd > rather wrote it for (int i = ...). Up to you. > > http://codereview.chromium.org/1594017/diff/8001/9003#newcode365 > test/cctest/test-strings.cc:365: CHECK_EQ(stringA[i], buffer[i]); > nit: current v8 styles is to either keep it in one line---for (...) > CHECK_EQ(...) or put into block: > > for (...) { > CHECK_EQ(...); > } > > http://codereview.chromium.org/1594017/diff/8001/9003#newcode368 > test/cctest/test-strings.cc:368: CHECK_EQ(stringB[i], buffer[i+3]); > ditto > > http://codereview.chromium.org/1594017 > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev To unsubscribe, reply using "remove me" as the subject.
