Cleanup StringCharacterStream and add initial test cases.

Cleanup StringCharacterStream and add initial test cases.

BUG=

Committed: https://code.google.com/p/v8/source/detail?r=13189

[drcarney, 2012-12-06 13:21:05]

switched a bunch of variable cases
added back some of the test
optimized away the trace_ variable

[Yang, 2012-12-10 13:45:29]

LGTM, but I have a bunch of comments. A lot of them are formatting-related, some
are simply suggestions.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings.cc
File test/cctest/test-strings.cc (right):

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:19: class RandomNumberGenerator {
You could also make a thin wrapper around v8's RNG. See v8.cc, seed_random,
random_base and V8::Random. I'd be fine with this implementation too though.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:32: for (unsigned j = 3; j < kQSize; j++)
add brackets if the body of for is not on the same line.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:57: return true;
no line breaks for single-line if.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:140: slice_head_chars = rng->next(15);
you could avoid the while loop if you just add 1 to the random number.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:157: buf[j] = rng->next(65536);
I'd prefer (1 << 16) or 0x10000, but this is not what you changed to begin
with... just saying... :)

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:190: case 3: {
I don't see what the difference between case 1 and case 3 is besides stack and
heap allocation for the string content (before being copied onto V8's heap in
either case). Maybe we should get rid of one? If we keep the stack allocated
version, could we assert that len < 2000? Same goes for case 0.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:402: static void VerifyEqual(const ConsStringStats&
a, const ConsStringStats& b) {
This could be made a method of ConsStringStats.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:458: if (StringShape(cons_string->first()).IsCons())
{
I guess !cons_string->first()->IsConsString() works just as well (actually it's
a wrapper for the StringShape check). Here and elsewhere.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:495: &response.length_);
Please format this so that the first argument is in the first line, and the
remaining arguments align with the first.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:505: return;
no line break here.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:511: break;
no line break here.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:540: ConsStringGenerationData* data,
you could move the first argument to the first line, and align the second
argument to it.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:597: int testCase,
first argument on first line, second aligned to first.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:602: data->rng_.next();
same line or brackets.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:618: default:
indentation.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:628: #ifndef NDEBUG
maybe you mean #ifdef DEBUG?

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:651: int outer_iterations = length >= 103 ? 103 :
30;
Why do we need to repeat this test so many times?

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:653: int offset =
(double)length*(double)j/(double)outer_iterations; // NOLINT
We always use static_cast, reinterpret_cast, etc. I guess here it would be
static_cast<double>. Lint would agree.

[drcarney, 2012-12-11 10:08:24]

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings.cc
File test/cctest/test-strings.cc (right):

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:19: class RandomNumberGenerator {
On 2012/12/10 13:45:29, Yang wrote:
> You could also make a thin wrapper around v8's RNG. See v8.cc, seed_random,
> random_base and V8::Random. I'd be fine with this implementation too though.

Okay. I'll clean it up in another cl. I see that the contents of seed_random are
copied all over the place in the test dir.  Maybe I can put a rng class in
cctest.h? Incidentally, this implementation is actually slightly better than the
fast implementation in v8.cc, which is why I went with it.

https://codereview.chromium.org/11438046/diff/13002/test/cctest/test-strings....
test/cctest/test-strings.cc:140: slice_head_chars = rng->next(15);
On 2012/12/10 13:45:29, Yang wrote:
> you could avoid the while loop if you just add 1 to the random number.

I want to allow at most one of the slices to be zero, but if they are both set
it doesn't matter.