Add StringBufferStream

Add StringBufferStream

add a class StringBufferStream which will replace
StringInputBuffer and SafeStringInputBuffer and requires no
ascii/two byte encoding scheme

R=yangguo@chromium.org
BUG=

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

[Yang, 2012-12-04 14:49:11]

Mostly style changes, aside from the GC issue we talked about offline.

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2532
src/objects-inl.h:2532: case kSeqStringTag | kOneByteStringTag:
We usually indent switch-case-clauses like
switch (x) {
  case y:
    z();
    break;
}

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2536
src/objects-inl.h:2536: length - offset);
I suggest adding four more spaces as indentation.

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2574
src/objects-inl.h:2574: if (string == NULL)
no line break here.

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2805
src/objects-inl.h:2805: maximumDepth = depth;
No line break here.

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2849
src/objects-inl.h:2849: return false;
no line break.

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2865
src/objects-inl.h:2865: void StringBufferStream::Reset(String* string, unsigned
offset,
Please put all arguments on the second line.

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2878
src/objects-inl.h:2878: return true;
no line break here and in the following if.

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2894
src/objects-inl.h:2894: unsigned length) {
Either align the two arguments or put both on the second line.

https://codereview.chromium.org/11428106/diff/2001/src/objects-inl.h#newcode2902
src/objects-inl.h:2902: unsigned length) {
Ditto.

https://codereview.chromium.org/11428106/diff/2001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11428106/diff/2001/src/objects.cc#newcode7035
src/objects.cc:7035: int32_t type;
the instance type is a byte value, so uint8_t would be enough.

https://codereview.chromium.org/11428106/diff/2001/src/objects.cc#newcode7035
src/objects.cc:7035: int32_t type;
Generally I wouldn't mind sprinkling ASSERTs around, e.g.
ASSERT(targetOffset >= offset);

https://codereview.chromium.org/11428106/diff/2001/src/objects.cc#newcode7040
src/objects.cc:7040: // Keep going if we're still in a ConString.
typo "ConsString"

https://codereview.chromium.org/11428106/diff/2001/src/objects.cc#newcode7051
src/objects.cc:7051: // Keep going if we're still in a ConString.
typo "ConsString"

https://codereview.chromium.org/11428106/diff/2001/src/objects.cc#newcode7106
src/objects.cc:7106: bool haveNotReadRight = trace & MaskForDepth(depth - 1);
Should this actually be hasAlreadyReadRight? (We go up the tree if the right
side of current cons string has already been traversed).

https://codereview.chromium.org/11428106/diff/2001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11428106/diff/2001/src/objects.h#newcode7510
src/objects.h:7510: static inline void Visit(String* string,
Why is this part of the String class and not part of the StringBufferStream?

https://codereview.chromium.org/11428106/diff/2001/src/objects.h#newcode7997
src/objects.h:7997: static const unsigned stackSize = 32;
Please write constants in camel case starting with k, like kStackSize.

https://codereview.chromium.org/11428106/diff/2001/src/objects.h#newcode7999
src/objects.h:7999: static const unsigned depthMask = 0x1F;
Could you write that as stackSize - 1 and assert that
STATIC_ASSERT(IS_POWER_OF_TWO(stackSize))?

https://codereview.chromium.org/11428106/diff/2001/src/objects.h#newcode8012
src/objects.h:8012: unsigned depth;
Please add an underscore to all member variables, like depth_, and use
underscore notation, like root_type_

https://codereview.chromium.org/11428106/diff/2001/src/objects.h#newcode8015
src/objects.h:8015: ConsString* frames[stackSize];
I wonder if you could just use a ZoneList to allocate a list of cons strings in
the Zone (things in the Zone are automatically deleted if the ZoneScope is left)
instead of a fixed-size stack.

https://codereview.chromium.org/11428106/diff/2001/src/objects.h#newcode8024
src/objects.h:8024: class StringBufferStream {
Different than what the name suggests, this class does not really offer a buffer
to read from, but rather hides traversing of a cons string, right?
I wonder how that would impact the performance (copying string content to a
linear buffer and reading that buffered chunk without interruptions vs.
traversing a cons string on demand). Unless of course you are planning to build
a buffered layer in between. So I'd suggest a rename.

https://codereview.chromium.org/11428106/diff/2001/src/objects.h#newcode8027
src/objects.h:8027: inline StringBufferStream() {}
I think it would be less error prone if the constructor implicitly calls Reset
to initialize member variables (which should use underscore notation and end
with an underscore).

https://codereview.chromium.org/11428106/diff/2001/src/objects.h#newcode8038
src/objects.h:8038: const uint16_t* buffer16;
Instead of using a pointer into the string, using an index relative to the
string start to address the current character would remove the use of an union
and also work if the string is being moved due to GC.

Same goes for end.

[Yang, 2012-12-06 10:18:47]

LGTM with one nit.

https://codereview.chromium.org/11428106/diff/9001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11428106/diff/9001/src/objects.cc#newcode7101
src/objects.cc:7101: bool haveNotReadRight = trace_ & MaskForDepth(depth_ - 1);
I'm still somewhat confused about this. Shouldn't it be called
hasAlreadyReadRight if the bit is set? We only continue with going right if we
haven't gone right yet.