Create an abstraction for the string type flags so that they can be cached....

Create an abstraction for the string type flags so that they can be cached.
Read the objects.h change first to understand what's going on here.

Committed: http://code.google.com/p/v8/source/detail?r=675

[Mads Ager (chromium), 2008-11-03 08:45:48]

LGTM with a bunch of nits.  Does this lint?

http://codereview.chromium.org/9038/diff/1/11
File src/compiler.cc (right):

http://codereview.chromium.org/9038/diff/1/11#newcode160
Line 160: StringShape source_shape(*source);
Pull out source_length in a variable here as you do below?

http://codereview.chromium.org/9038/diff/1/9
File src/conversions.cc (right):

http://codereview.chromium.org/9038/diff/1/9#newcode126
Line 126: int len = strlen(other);
Introduced other_len and str_len?

http://codereview.chromium.org/9038/diff/1/23
File src/factory.cc (right):

http://codereview.chromium.org/9038/diff/1/23#newcode101
Line 101: second_shape), String);
Put String on a new line aligned with Heap?

http://codereview.chromium.org/9038/diff/1/6
File src/heap.cc (right):

http://codereview.chromium.org/9038/diff/1/6#newcode1349
Line 1349: Object* Heap::AllocateConsString(String* first, StringShape
first_shape, String* second, StringShape second_shape) {
Isn't this line too long?

http://codereview.chromium.org/9038/diff/1/6#newcode1454
Line 1454: buffer_shape = buffer;
Please make the conversion explicit here.

http://codereview.chromium.org/9038/diff/1/17
File src/objects-inl.h (right):

http://codereview.chromium.org/9038/diff/1/17#newcode137
Line 137: bool Object::IsSeqString() {
Is it too expensive to use StringShapes for these?  This is pretty hard to read
and it also seems error prone.

http://codereview.chromium.org/9038/diff/1/8
File src/objects.cc (right):

http://codereview.chromium.org/9038/diff/1/8#newcode2915
Line 2915: return true;
Doesn't it make sense to check that the representation tag makes sense?

http://codereview.chromium.org/9038/diff/1/13
File src/objects.h (right):

http://codereview.chromium.org/9038/diff/1/13#newcode3026
Line 3026: // string can potentially alter its shape.
Would it make sense to put in some debug code that adds a valid_ member to the
StringShape class and sets it to false when flattened?  Then each of the
operations can assert that the shape is valid which could potentially catch
nasty bugs.

http://codereview.chromium.org/9038/diff/1/13#newcode3033
Line 3033: class StringShape {
Are these intended to be only stack allocated?  In that case, you should use
BASE_EMBEDDED:

class StringShape BASE_EMBEDDED {

http://codereview.chromium.org/9038/diff/1/13#newcode3035
Line 3035: inline StringShape(String* s);
One-argument constructors should be explicit.

http://codereview.chromium.org/9038/diff/1/12
File src/runtime.cc (right):

http://codereview.chromium.org/9038/diff/1/12#newcode2158
Line 2158: StringShape sshape(source);
How about source_shape?

http://codereview.chromium.org/9038/diff/1/12#newcode2187
Line 2187: StringShape dshape(destination);
dest_shape or even destination_shape?

http://codereview.chromium.org/9038/diff/1/12#newcode2271
Line 2271: StringShape sshape(source);
source_shape?

http://codereview.chromium.org/9038/diff/1/12#newcode2293
Line 2293: StringShape dshape(destination);
destination_shape?

[Erik Corry, 2008-11-03 09:33:54]

Thanks for the feedback.

http://codereview.chromium.org/9038/diff/1/8
File src/objects.cc (right):

http://codereview.chromium.org/9038/diff/1/8#newcode2915
Line 2915: return true;
On 2008/11/03 08:45:49, Mads Ager wrote:
> Doesn't it make sense to check that the representation tag makes sense?

It makes no sense because all bit combinations are valid.

http://codereview.chromium.org/9038/diff/1/13
File src/objects.h (right):

http://codereview.chromium.org/9038/diff/1/13#newcode3026
Line 3026: // string can potentially alter its shape.
On 2008/11/03 08:45:49, Mads Ager wrote:
> Would it make sense to put in some debug code that adds a valid_ member to the
> StringShape class and sets it to false when flattened?  Then each of the
> operations can assert that the shape is valid which could potentially catch
> nasty bugs.

At the moment the flatten operation doesn't actually change the shape, though
this is in the works :-)  I put in the scaffolding for the check, but at the
moment it's not used.

http://codereview.chromium.org/9038/diff/1/12
File src/runtime.cc (right):

http://codereview.chromium.org/9038/diff/1/12#newcode2187
Line 2187: StringShape dshape(destination);
On 2008/11/03 08:45:49, Mads Ager wrote:
> dest_shape or even destination_shape?

I changed the other one, but I left this in, because the 80 character limit
means that this code gets very much less readable with dest_shape.