- Represent strings internally in UTF-16 format, this makes it

- Represent strings internally in UTF-16 format, this makes it
  compatible with webkit and will allow for easy externalization of strings
  (The language specification was changed recently to reflect this as
   follows "A string is a sequence of UTF-16 code units").
- Remove four byte string class and all references to it.
- Rename some of the string functions in Dart API to make them consistent
  and better describe the underlying functionality
  Dart_NewString => Dart_NewStringFromCString
  Dart_NewString8 => Dart_NewStringFromUTF8
  Dart_NewString16 => Dart_NewStringFromUTF16
  Dart_NewString32 => Dart_NewStringFromUTF32
  Dart_NewExternalString8 => Dart_NewExternalUTF8String
  Dart_NewExternalString16 => Dart_NewExternalUTF16String
  Dart_NewExternalString32 => Dart_NewExternalUTF32String
  Dart_StringGet8 => Dart_StringToUTF8
  Dart_StringGet16 => Dart_StringToUTF16
  Dart_StringToCString => Dart_StringToCString
  Dart_IsString8 => Removed
  Dart_IsString16 -> Removed
  Dart_StringToBytes -> Removed
  Dart_StringGet32 -> Removed

[cshapiro, 2012-10-24 23:52:29]

lgtm with comments

I noticed you changed StringTo to StringAs which is something I was going to do
but, Todd though better of that convention to keep consistent with other
functions.  I would preserve the use of To (instead of As) for that reason.

https://chromiumcodereview.appspot.com/11275008/diff/3011/bin/dartutils.cc
File bin/dartutils.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/bin/dartutils.cc#ne...
bin/dartutils.cc:108: const char* name) {
Not sure what caused this change.  Either way, the indentation looks off.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/benchmark_test.cc
File vm/benchmark_test.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/benchmark_test.c...
vm/benchmark_test.cc:183: Dart_Invoke(lib, NewString("benchmark"), 1, args);
Replace this with the macro expansion?

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/benchmark_test.h
File vm/benchmark_test.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/benchmark_test.h...
vm/benchmark_test.h:38: #define NewString(str) Dart_NewStringFromCString(str)
This macro seems unnecessary.  Make this a static or inline function instead? 
Or, just do a string replace?  

Either way, we would be in closer agreement with the spirit style guide.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/object_test.cc
File vm/object_test.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/object_test.cc#n...
vm/object_test.cc:1259: // Concatenate 1byte, 2byte and 4byte strings.
Not sure why this was changed.  The rules of grammar suggest the original
version is correct.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/raw_object.h
File vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/raw_object.h#new...
vm/raw_object.h:1514: 
If you are now in the business of adding missing white space this line is a
missed opportunity.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/raw_object.h#new...
vm/raw_object.h:1524: 
Ditto.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc
File vm/unicode.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:13: static const int8_t kTrailBytes[256] = {
If you are going to mess with this table at all we should fold it in half and
index by "val >> 1".

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:61: static bool IsIsoLatin1(uint8_t code_unit) {
See below.  This function and the one below it sniff the first code unit of a
(potentially) multi-code-unit sequence.  They should be named as such.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:67: static bool IsSMP(uint8_t code_unit) {
I think this is a somewhat sketchy name.  This code unit is the first byte of a
supplementary character.  Why not call this IsSmpFirstByte or IsSmpStart or some
other such thing?

Also, these are only used by one routine.  Why not move them next to it?

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:92: ASSERT(codepoint >= 0x10000);
I think you need a constant for kMaxBmpCodePoint

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:224: uint32_t ch = utf8_array[0] & 0xFF;
The & is probably unnecessary now as the lhs and rhs are both unsigned.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:227: int32_t num_trail_bytes = kTrailBytes[ch];
This has no significance as an int32, why not just an int?

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:268: ASSERT(ch <= 0xff);
Replace 0xFF with kMaxOneByteCharacter

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.h
File vm/unicode.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.h#newcode18
vm/unicode.h:18: kISOLatin1 = 0,  // ISO Latin-1 character set.
Any reason for the "= 0" here?  Do we care about the numbering?

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.h#newcode22
vm/unicode.h:22: 
Adding constants for kMaxCodePoint and kMaxBmpCodePoint would allow other
constants to go away in unicode.cc.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.h#newcode35
vm/unicode.h:35: // Returns true if C string is a valid UTF-8 string.
...true if src is a...

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unit_test.h
File vm/unit_test.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unit_test.h#newc...
vm/unit_test.h:135: #define NewString(str) Dart_NewStringFromCString(str)
Why not just rename this function globally?

[Anton Muhin, 2012-10-25 13:35:47]

https://chromiumcodereview.appspot.com/11275008/diff/3011/include/dart_api.h
File include/dart_api.h (left):

https://chromiumcodereview.appspot.com/11275008/diff/3011/include/dart_api.h#...
include/dart_api.h:1329: DART_EXPORT bool Dart_IsString8(Dart_Handle object);
Internally, are all the strings utf16 now?

I am asking as actually currently WebKit supports both utf16 and ascii strings
and I'd prefer to have (eventually) fast paths for both cases which escape
decoding.

[Tom Ball, 2012-10-25 21:29:43]

lgtm

[siva, 2012-10-26 21:38:29]

- Addressed review comments.
- retained names Dart_StringToCString, DartStringToUTF8 and Dart_StringToUTF16

This change triggers some changes in dart2js so I am going to move this change
to another workspace that is based on dart-all.

https://chromiumcodereview.appspot.com/11275008/diff/3011/bin/dartutils.cc
File bin/dartutils.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/bin/dartutils.cc#ne...
bin/dartutils.cc:108: const char* name) {
There was an indentation issue before which I fixed, not
sure why it shows up like this in the diff.

On 2012/10/24 23:52:29, cshapiro wrote:
> Not sure what caused this change.  Either way, the indentation looks off.

https://chromiumcodereview.appspot.com/11275008/diff/3011/include/dart_api.h
File include/dart_api.h (left):

https://chromiumcodereview.appspot.com/11275008/diff/3011/include/dart_api.h#...
include/dart_api.h:1329: DART_EXPORT bool Dart_IsString8(Dart_Handle object);
No we still retain the one byte string representation in the VM, so all ascii
strings are represented with just 1 byte per character.

On 2012/10/25 13:35:48, Anton Muhin wrote:
> Internally, are all the strings utf16 now?
> 
> I am asking as actually currently WebKit supports both utf16 and ascii strings
> and I'd prefer to have (eventually) fast paths for both cases which escape
> decoding.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/benchmark_test.cc
File vm/benchmark_test.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/benchmark_test.c...
vm/benchmark_test.cc:183: Dart_Invoke(lib, NewString("benchmark"), 1, args);
NewString is now a method.

On 2012/10/24 23:52:29, cshapiro wrote:
> Replace this with the macro expansion?

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/benchmark_test.h
File vm/benchmark_test.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/benchmark_test.h...
vm/benchmark_test.h:38: #define NewString(str) Dart_NewStringFromCString(str)
Made an inline function doing a string replace results
in too many indentation issues.

On 2012/10/24 23:52:29, cshapiro wrote:
> This macro seems unnecessary.  Make this a static or inline function instead? 
> Or, just do a string replace?  
> 
> Either way, we would be in closer agreement with the spirit style guide.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/object_test.cc
File vm/object_test.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/object_test.cc#n...
vm/object_test.cc:1259: // Concatenate 1byte, 2byte and 4byte strings.
Restored to original format.

On 2012/10/24 23:52:29, cshapiro wrote:
> Not sure why this was changed.  The rules of grammar suggest the original
> version is correct.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/raw_object.h
File vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/raw_object.h#new...
vm/raw_object.h:1514: 
I am not in that business (doesn't pay much), but I will
do this one :-)

On 2012/10/24 23:52:29, cshapiro wrote:
> If you are now in the business of adding missing white space this line is a
> missed opportunity.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/raw_object.h#new...
vm/raw_object.h:1524: 
On 2012/10/24 23:52:29, cshapiro wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc
File vm/unicode.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:13: static const int8_t kTrailBytes[256] = {
maybe for another CL.

On 2012/10/24 23:52:29, cshapiro wrote:
> If you are going to mess with this table at all we should fold it in half and
> index by "val >> 1".

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:61: static bool IsIsoLatin1(uint8_t code_unit) {
Renamed to IsIsoLatin1SequenceStart but left it here as we generally collect all
static functions at the top of the file.

On 2012/10/24 23:52:29, cshapiro wrote:
> See below.  This function and the one below it sniff the first code unit of a
> (potentially) multi-code-unit sequence.  They should be named as such.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:67: static bool IsSMP(uint8_t code_unit) {
Renamed to IsSmpSequenceStart but left it here as we generally collect all
static functions at the top of the file.

On 2012/10/24 23:52:29, cshapiro wrote:
> I think this is a somewhat sketchy name.  This code unit is the first byte of
a
> supplementary character.  Why not call this IsSmpFirstByte or IsSmpStart or
some
> other such thing?
> 
> Also, these are only used by one routine.  Why not move them next to it?

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:92: ASSERT(codepoint >= 0x10000);
On 2012/10/24 23:52:29, cshapiro wrote:
> I think you need a constant for kMaxBmpCodePoint

Done.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:224: uint32_t ch = utf8_array[0] & 0xFF;
On 2012/10/24 23:52:29, cshapiro wrote:
> The & is probably unnecessary now as the lhs and rhs are both unsigned.

Done.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:227: int32_t num_trail_bytes = kTrailBytes[ch];
Changed to int8_t to match the type of kTrailBytes

On 2012/10/24 23:52:29, cshapiro wrote:
> This has no significance as an int32, why not just an int?

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.cc#newco...
vm/unicode.cc:268: ASSERT(ch <= 0xff);
As discussed offline this is 0xff unless we decide to
go with ASCII only characters for OneByteString

On 2012/10/24 23:52:29, cshapiro wrote:
> Replace 0xFF with kMaxOneByteCharacter

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.h
File vm/unicode.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.h#newcode18
vm/unicode.h:18: kISOLatin1 = 0,  // ISO Latin-1 character set.
No just following general style we have used for enums.

On 2012/10/24 23:52:29, cshapiro wrote:
> Any reason for the "= 0" here?  Do we care about the numbering?

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.h#newcode22
vm/unicode.h:22: 
On 2012/10/24 23:52:29, cshapiro wrote:
> Adding constants for kMaxCodePoint and kMaxBmpCodePoint would allow other
> constants to go away in unicode.cc.

Done.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unicode.h#newcode35
vm/unicode.h:35: // Returns true if C string is a valid UTF-8 string.
On 2012/10/24 23:52:29, cshapiro wrote:
> ...true if src is a...

Done.

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unit_test.h
File vm/unit_test.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/3011/vm/unit_test.h#newc...
vm/unit_test.h:135: #define NewString(str) Dart_NewStringFromCString(str)
Causes too many indentation issues and since most are just test files I did not
want to go and change all of it.

Made this an inline function here instead of a macro.

On 2012/10/24 23:52:29, cshapiro wrote:
> Why not just rename this function globally?

[Anton Muhin, 2012-10-29 16:05:45]

https://chromiumcodereview.appspot.com/11275008/diff/29006/bin/dartutils.cc
File bin/dartutils.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/29006/bin/dartutils.cc#n...
bin/dartutils.cc:216: delete file;
shouldn't you deallocate text_buffer here too?

https://chromiumcodereview.appspot.com/11275008/diff/29006/include/dart_api.h
File include/dart_api.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/29006/include/dart_api.h...
include/dart_api.h:1475: DART_EXPORT Dart_Handle Dart_StringToUTF16(Dart_Handle
str,
there seems to be no way to find out optimal target encoding of the string.

https://chromiumcodereview.appspot.com/11275008/diff/29006/vm/object.cc
File vm/object.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/29006/vm/object.cc#newco...
vm/object.cc:10258: memmove(utf8_array, obj.CharAddr(0), obj.Length());
I believe you just forbid memmove's in VM, but memcpy might be slightly more
efficient in this case as if data overlap, we're already in a big trouble.

https://chromiumcodereview.appspot.com/11275008/diff/29006/vm/object.cc#newco...
vm/object.cc:10442: RawOneByteString* OneByteString::New(const uint32_t*
characters,
do you still need this factory?

[siva, 2012-10-30 00:33:34]

https://chromiumcodereview.appspot.com/11275008/diff/29006/bin/dartutils.cc
File bin/dartutils.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/29006/bin/dartutils.cc#n...
bin/dartutils.cc:216: delete file;
On 2012/10/29 16:05:46, Anton Muhin wrote:
> shouldn't you deallocate text_buffer here too?

Good point, added a free(text_buffer).

https://chromiumcodereview.appspot.com/11275008/diff/29006/include/dart_api.h
File include/dart_api.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/29006/include/dart_api.h...
include/dart_api.h:1475: DART_EXPORT Dart_Handle Dart_StringToUTF16(Dart_Handle
str,
Not sure I understand the comment, these two functions
Dart_StringToUTF8 and Dart_StringToUTF16 should work
for both one byte and two byte strings.

If you are referring to the missing Dart_IsAsciiString
functionality, yes I will add it.

On 2012/10/29 16:05:46, Anton Muhin wrote:
> there seems to be no way to find out optimal target encoding of the string.

https://chromiumcodereview.appspot.com/11275008/diff/29006/vm/object.cc
File vm/object.cc (right):

https://chromiumcodereview.appspot.com/11275008/diff/29006/vm/object.cc#newco...
vm/object.cc:10258: memmove(utf8_array, obj.CharAddr(0), obj.Length());
Yes in platform/globals.h we forbid memcpy in the VM.
We can probably consider relaxing this in another CL.

On 2012/10/29 16:05:46, Anton Muhin wrote:
> I believe you just forbid memmove's in VM, but memcpy might be slightly more
> efficient in this case as if data overlap, we're already in a big trouble.

https://chromiumcodereview.appspot.com/11275008/diff/29006/vm/object.cc#newco...
vm/object.cc:10442: RawOneByteString* OneByteString::New(const uint32_t*
characters,
Yes it is used from Dart_NewStringFromUTF32.

On 2012/10/29 16:05:46, Anton Muhin wrote:
> do you still need this factory?

[Anton Muhin, 2012-10-30 08:40:44]

lgtm

https://chromiumcodereview.appspot.com/11275008/diff/29006/include/dart_api.h
File include/dart_api.h (right):

https://chromiumcodereview.appspot.com/11275008/diff/29006/include/dart_api.h...
include/dart_api.h:1475: DART_EXPORT Dart_Handle Dart_StringToUTF16(Dart_Handle
str,
Dart_IsAscii is all I need, thanks a lot, Siva.

On 2012/10/30 00:33:34, siva wrote:
> Not sure I understand the comment, these two functions
> Dart_StringToUTF8 and Dart_StringToUTF16 should work
> for both one byte and two byte strings.
> 
> If you are referring to the missing Dart_IsAsciiString
> functionality, yes I will add it.
> 
> On 2012/10/29 16:05:46, Anton Muhin wrote:
> > there seems to be no way to find out optimal target encoding of the string.
>

[Anton Muhin, 2012-11-12 13:28:13]

Is it obsolete now?