Add some support for the code-point code-unit distinction.

Add some support for the code-point code-unit distinction.

Adds String.fromCodeUnits and String.codeUnitAt, and modifies
String.fromCodePoints and String.codePointAt to actually use code points.
Fixes String.charCodes to use code points and adds String.codeUnits.
Reenables some tests, and adds new ones for non-BMP characters.
Fixes issues 6418, 6501 and 1357.

R=floitsch@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=15150

[erikcorry, 2012-11-08 10:38:55]

See also http://code.google.com/p/co19/issues/detail?id=284

[floitsch, 2012-11-08 15:28:21]

LGTM.
Please wait for the VM guys to comment on the signed-to-unsigned change.
Added Peter (because it slightly touches the scanner).

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode32
runtime/lib/string.cc:32: uint32_t value = Smi::Cast(index_object).Value();
I don't think this works.
On 64bit machines a Smi has 63 bits. Even with an explicit cast you would lose
some bits, thus making the "value > Utf16::kMaxCodePoint" below not throw on
some big values.

If IsSurrogate expects a uint32_t, cast it there.

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode71
runtime/lib/string.cc:71: uint32_t value = Smi::Cast(index_object).Value();
ditto.

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode161
runtime/lib/string.cc:161: if (value <= Utf16::kMaxCodeUnit) {
As discussed: This should just return a String with the code-unit at the offset.
Ie the string-equivalent of String.codeUnitAt.

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string_base.dart
File runtime/lib/string_base.dart (right):

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string_base.dart#...
runtime/lib/string_base.dart:326: if (c > 0xffff) {
Can't you use Utf16::IsSurrogate(c)?

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h#newcode4122
runtime/vm/object.h:4122: int32_t unit = *CharAddr(str, index);
uint32_t unit =

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:139:
if (code >= 0x10000) {
Why not move the surrogate-code into addHexEscape?

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:669: result[i] =
JS('int', r'#.charCodeAt(#)', receiver, i);
why not
result[i] = receiver.codeUnitAt(i);
?

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:546: static String
stringFromCharCodes(charCodes) {
I would move this code to the patch-class and redirect to fromCodeUnits there.
Maybe ask the others first what they think.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/scanner/token.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/scanner/token.dart:249: int next() {
maybe add TODO to switch to charcode-iterator once we have one.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/tree/dartstring.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/dartstring.dart:102: int code =
it.next();
leave as is?

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:29: * If the index corresponds to the first of two
UTF-16 surrogate pair
Update comment.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:37: * pairs are handled as in [operator []].
Update comment.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:42: * Gets the 16 bit UTF-16 code unit at the given
index.
Update comment (now same as [operator []]).

[Ivan Posva, 2012-11-08 18:20:38]

Do not commit this before you get buy-in from the VM team.

Thanks,
-Ivan

[siva, 2012-11-08 19:06:47]

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h#newcode3968
runtime/vm/object.h:3968: }
I don't mind changing the  name 'CharAt' to 'CodeUnitAt" but
I don't think your new 'CharAt' functionality belongs
in the VM code.

Iteration of characters in a string should be done in an
Iterator implementation in the library.

[erikcorry, 2012-11-08 22:09:34]

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h#newcode3968
runtime/vm/object.h:3968: }
On 2012/11/08 19:06:47, siva wrote:
> I don't mind changing the  name 'CharAt' to 'CodeUnitAt" but
> I don't think your new 'CharAt' functionality belongs
> in the VM code.
> 
> Iteration of characters in a string should be done in an
> Iterator implementation in the library.

Do you want to move the C++ support for toUpperCase and toLowerCase into dart
too?  They make use of charAt.

[siva, 2012-11-09 02:40:42]

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h#newcode3968
runtime/vm/object.h:3968: }
We could fix String::Transform/TwoByteString::Transform
to locally compute the unicode values as it iterates over
the string.

On 2012/11/08 22:09:34, erikcorry wrote:
> On 2012/11/08 19:06:47, siva wrote:
> > I don't mind changing the  name 'CharAt' to 'CodeUnitAt" but
> > I don't think your new 'CharAt' functionality belongs
> > in the VM code.
> > 
> > Iteration of characters in a string should be done in an
> > Iterator implementation in the library.
> 
> Do you want to move the C++ support for toUpperCase and toLowerCase into dart
> too?  They make use of charAt.

[cshapiro, 2012-11-09 04:43:57]

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode162
runtime/lib/string.cc:162: return Symbols::New(&value, 1);
This is generally not a good idea as the intern table is not  garbage collected
(at least not yet).

[erikcorry, 2012-11-12 20:22:17]

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode162
runtime/lib/string.cc:162: return Symbols::New(&value, 1);
On 2012/11/09 04:43:57, cshapiro wrote:
> This is generally not a good idea as the intern table is not  garbage
collected
> (at least not yet).

It already interned character strings, I did not change that.

[cshapiro, 2012-11-12 21:51:13]

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode162
runtime/lib/string.cc:162: return Symbols::New(&value, 1);
Yes, what you say is all true.  This change is an opportunity to remove that
mistake.

[ngeoffray, 2012-11-15 13:24:20]

FYI

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/js_helper.dart:546: static String
stringFromCharCodes(charCodes) {
On 2012/11/08 15:28:21, floitsch wrote:
> I would move this code to the patch-class and redirect to fromCodeUnits there.
> Maybe ask the others first what they think.

I don't think the patch classes have access to 'JS', so I would keep the method
where it is. The less libraries that touch JS, the better.

[erikcorry, 2012-11-15 13:28:24]

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode32
runtime/lib/string.cc:32: uint32_t value = Smi::Cast(index_object).Value();
On 2012/11/08 15:28:21, floitsch wrote:
> I don't think this works.
> On 64bit machines a Smi has 63 bits. Even with an explicit cast you would lose
> some bits, thus making the "value > Utf16::kMaxCodePoint" below not throw on
> some big values.
> 
> If IsSurrogate expects a uint32_t, cast it there.

Done.

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode71
runtime/lib/string.cc:71: uint32_t value = Smi::Cast(index_object).Value();
On 2012/11/08 15:28:21, floitsch wrote:
> ditto.
 This code is now gone.

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode161
runtime/lib/string.cc:161: if (value <= Utf16::kMaxCodeUnit) {
On 2012/11/08 15:28:21, floitsch wrote:
> As discussed: This should just return a String with the code-unit at the
offset.
> Ie the string-equivalent of String.codeUnitAt.

Done.

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string.cc#newcode162
runtime/lib/string.cc:162: return Symbols::New(&value, 1);
On 2012/11/12 21:51:13, cshapiro wrote:
> Yes, what you say is all true.  This change is an opportunity to remove that
> mistake.

I fixed it so it only interns ASCII characters.

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string_base.dart
File runtime/lib/string_base.dart (right):

https://codereview.chromium.org/11368138/diff/1/runtime/lib/string_base.dart#...
runtime/lib/string_base.dart:326: if (c > 0xffff) {
On 2012/11/08 15:28:21, floitsch wrote:
> Can't you use Utf16::IsSurrogate(c)?

No, that's a C++ function.  I added some named constants to make this less ugly.

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h#newcode3968
runtime/vm/object.h:3968: }
On 2012/11/09 02:40:42, siva wrote:
> We could fix String::Transform/TwoByteString::Transform
> to locally compute the unicode values as it iterates over
> the string.

I have fixed the UTF8 and the Transform (toUpper/Lower) routines to use a helper
from unicode.h and removed this from the String classes.

> 
> On 2012/11/08 22:09:34, erikcorry wrote:
> > On 2012/11/08 19:06:47, siva wrote:
> > > I don't mind changing the  name 'CharAt' to 'CodeUnitAt" but
> > > I don't think your new 'CharAt' functionality belongs
> > > in the VM code.
> > > 
> > > Iteration of characters in a string should be done in an
> > > Iterator implementation in the library.
> > 
> > Do you want to move the C++ support for toUpperCase and toLowerCase into
dart
> > too?  They make use of charAt.
>

https://codereview.chromium.org/11368138/diff/1/runtime/vm/object.h#newcode4122
runtime/vm/object.h:4122: int32_t unit = *CharAddr(str, index);
On 2012/11/08 15:28:21, floitsch wrote:
> uint32_t unit =

This code is gone.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart
(right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/js_backend/constant_emitter.dart:139:
if (code >= 0x10000) {
On 2012/11/08 15:28:21, floitsch wrote:
> Why not move the surrogate-code into addHexEscape?

This part has all been rewritten with the new JSON outputting code.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/lib/interceptors.dart:669: result[i] =
JS('int', r'#.charCodeAt(#)', receiver, i);
On 2012/11/08 15:28:21, floitsch wrote:
> why not
> result[i] = receiver.codeUnitAt(i);
> ?

Unfortunately the optimizer can't optimize that away again:

-    t1 = receiver.charCodeAt(i);
+    t1 = $.getInterceptor(receiver).codeUnitAt$1(receiver, i);

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/scanner/token.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/scanner/token.dart:249: int next() {
On 2012/11/08 15:28:21, floitsch wrote:
> maybe add TODO to switch to charcode-iterator once we have one.

Done.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/tree/dartstring.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/tree/dartstring.dart:102: int code =
it.next();
On 2012/11/08 15:28:21, floitsch wrote:
> leave as is?

Done.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:29: * If the index corresponds to the first of two
UTF-16 surrogate pair
On 2012/11/08 15:28:21, floitsch wrote:
> Update comment.

Done.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:37: * pairs are handled as in [operator []].
On 2012/11/08 15:28:21, floitsch wrote:
> Update comment.

Done.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:42: * Gets the 16 bit UTF-16 code unit at the given
index.
On 2012/11/08 15:28:21, floitsch wrote:
> Update comment (now same as [operator []]).

Done.

https://codereview.chromium.org/11368138/diff/1/sdk/lib/core/string.dart#newc...
sdk/lib/core/string.dart:42: * Gets the 16 bit UTF-16 code unit at the given
index.
On 2012/11/08 15:28:21, floitsch wrote:
> Update comment (now same as [operator []]).

Done.

[erikcorry, 2012-11-15 14:55:41]

I uploaded a new version.

On the VM side it is less invasive.  The CodePointAt method has been moved off
the String class and into the Utf16 class as requested.

[cshapiro, 2012-11-15 20:14:51]

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/regexp_jsc.cc
File runtime/lib/regexp_jsc.cc (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/regexp_jsc.cc...
runtime/lib/regexp_jsc.cc:22: two_byte_str[i] = str.CodeUnitAt(i);
Please do not change the name of this method on the VM string class.

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc
File runtime/lib/string.cc (left):

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc#old...
runtime/lib/string.cc:104: ASSERT(value <= 0x10FFFF);
I am not sure why this ASSERT was removed.  It is still relevant in the new
version.

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc#new...
runtime/lib/string.cc:14: DEFINE_NATIVE_ENTRY(StringBase_createFromCodeUnits, 1)
{
I might have missed something, but it seemed clear to me that we would not
introduce the name "code units" in the API as it is inherently ambiguous.

This should be called "createFromUTF16" or some such thing.

Moreover, I would have expected this function (createFromCodePoints) to have
been renamed to createFromUTF32.

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc#new...
runtime/lib/string.cc:101: if (value <= 0x7f) {
Can we name this constant?

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string_base.dart
File runtime/lib/string_base.dart (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string_base.d...
runtime/lib/string_base.dart:18: static String createFromCodeUnits(List<int>
codeUnits) {
Please stick with naming this by the input type.  In this case, UTF-16.

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:144: if (name.CodeUnitAt(i) == ':') {
This file is an example of why we would like the rename and unsigned type
changes to be removed from this change.

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/object.h#newco...
runtime/vm/object.h:3892: static uint32_t CodeUnitAt(const String& str, intptr_t
index) {
Why did the signedness of the return type change?  The values are maximally in
the 21-bit range.  Given how we use signed types pervasively in the VM this is
going to introduce opportunities for signedness mismatch warnings in the
compiler.

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/unicode.h
File runtime/vm/unicode.h (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/unicode.h#newc...
runtime/vm/unicode.h:77: static const uint32_t kInvalidCodePoint =  0xffffffffu;
can we keep the casing of these values consistent?  This file mostly uses all
upper...

[erikcorry, 2012-11-15 23:47:05]

New version uploaded

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/regexp_jsc.cc
File runtime/lib/regexp_jsc.cc (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/regexp_jsc.cc...
runtime/lib/regexp_jsc.cc:22: two_byte_str[i] = str.CodeUnitAt(i);
On 2012/11/15 20:14:51, cshapiro wrote:
> Please do not change the name of this method on the VM string class.

The strings are now made up of UTF16 code units, not characters.  However I have
renamed it since you prefer the old name.

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc
File runtime/lib/string.cc (left):

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc#old...
runtime/lib/string.cc:104: ASSERT(value <= 0x10FFFF);
On 2012/11/15 20:14:51, cshapiro wrote:
> I am not sure why this ASSERT was removed.  It is still relevant in the new
> version.

In the new version the value is always a UTF-16 code unit.  I have inserted an
assert that it is always <= 0xffff.

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc#new...
runtime/lib/string.cc:14: DEFINE_NATIVE_ENTRY(StringBase_createFromCodeUnits, 1)
{
On 2012/11/15 20:14:51, cshapiro wrote:
> I might have missed something, but it seemed clear to me that we would not
> introduce the name "code units" in the API as it is inherently ambiguous.
> 
> This should be called "createFromUTF16" or some such thing.

Done.

> Moreover, I would have expected this function (createFromCodePoints) to have
> been renamed to createFromUTF32.

No, this one creates from UTF16 code units.

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string.cc#new...
runtime/lib/string.cc:101: if (value <= 0x7f) {
On 2012/11/15 20:14:51, cshapiro wrote:
> Can we name this constant?

kFrequentCharacterLimit.

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string_base.dart
File runtime/lib/string_base.dart (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/lib/string_base.d...
runtime/lib/string_base.dart:18: static String createFromCodeUnits(List<int>
codeUnits) {
On 2012/11/15 20:14:51, cshapiro wrote:
> Please stick with naming this by the input type.  In this case, UTF-16.

Done.

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:144: if (name.CodeUnitAt(i) == ':') {
On 2012/11/15 20:14:51, cshapiro wrote:
> This file is an example of why we would like the rename and unsigned type
> changes to be removed from this change.

Done.

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/object.h#newco...
runtime/vm/object.h:3892: static uint32_t CodeUnitAt(const String& str, intptr_t
index) {
On 2012/11/15 20:14:51, cshapiro wrote:
> Why did the signedness of the return type change?  The values are maximally in
> the 21-bit range.  Given how we use signed types pervasively in the VM this is
> going to introduce opportunities for signedness mismatch warnings in the
> compiler.

There is a mixture in the VM of signed and unsigned values for the strings.  For
example the data_ and external_data_ fields on raw strings are unsigned.  I was
getting signedness mismatch warnings from the compiler so I decided to
standardize on unsigned values.  In response to this comment here I have tried
standardizing on signed values, but that causes cascades of  changes throughout
the VM.  In the next patch set I have therefore used a mixture, but with a
tendency towards using int32_t where the values are 32 bit and unsigned types
for the 16 bit and 8 bit values.  This function (now named CharAt) is thus
signed again.

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/unicode.h
File runtime/vm/unicode.h (right):

https://codereview.chromium.org/11368138/diff/12001/runtime/vm/unicode.h#newc...
runtime/vm/unicode.h:77: static const uint32_t kInvalidCodePoint =  0xffffffffu;
On 2012/11/15 20:14:51, cshapiro wrote:
> can we keep the casing of these values consistent?  This file mostly uses all
> upper...

Done.

[siva, 2012-11-16 22:32:04]

It would have been nice if we implemented the
Codepoint iterator class first in Dart and then changed
the library code to use the iterator instead of this
intermediate step which uses charCodeAt.

Similarly in the C++ code Carl is writing a
CodepointIterator and that will be used in the C++ code.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.dart
File runtime/lib/string_base.dart (right):

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:18: static String createFromUtf16(List<int>
codeUnits) {
The comment
"Create the .... for specified UTF-16 code units" would
still be valid right ?

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:315: return newResult;
This piece of code is repeated 3 times in this CL, why not abstract it out.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:389: class _TwoByteStringBase extends _StringBase {
Maybe add a TODO here to get rid of this class and use
mixins instead once they are implemented in dart.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:429: List<int> get charCodes {
TODO, fix this to use the new code point iterator when it is available?

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:442: if (i == j) return result;
I find the (i == j) condition here a little unreadable,
why not introduce a boolean variable non_bmp_char_seen
and test for that.

The same comment also holds for the splitChars case.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:450: }
This code here and in splitChars above seem to be identical,
it might make sense to abstract it out (maybe a static
method that is passed in a string object).

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:462: // TODO(erikcorry): This is going away.
(why not remove it in this CL itself)?

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:507: // TODO(erikcorry): This is going away.
Ditto comment.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_patch....
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_patch....
runtime/lib/string_patch.dart:7: return _StringBase.createFromUtf16(codeUnits);
I think we have consistently used the same name when
patching in functions in order to avoid confusion, why
not call it String.createFromUtf16

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10017: }
I don't quite get this implementation.

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10803: ASSERT(ch <= Utf16::kMaxCodePoint);
We assert here that ch <= Utf16::kMaxCodePoint
but we have code below that deals with the case when
ch > Utf16::kMaxCodePoint

The ASSERT should be removed?

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.h#newco...
runtime/vm/object.h:3724: // always use this if you are looking for ASCII.  If
you need to support
Should we say ASCII or Latin1 here?

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.h#newco...
runtime/vm/object.h:3950: int out_length,
we always use intptr_t for length values in the VM

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.h#newco...
runtime/vm/object.h:4034: int out_length,
Ditto.

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:30: factory String.fromCharCodes(List<int> charCodes) {
why not call this createFromUtf32

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:69: external factory String.fromCodeUnits(List<int>
codeUnits);
Should be called  createFromUtf16 to avoid confusion
you have already renamed the VM method.

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:192: List<int> get charCodes {
TODO use codepoint iterator instead of charCodeAt here?

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:211: return newResult;
This piece of code was repeated earlier too, maybe abstract
it out.

[cshapiro, 2012-11-17 02:25:27]

http://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:9895: return OneByteString::CharAt(*this, index);
Why was this changed?  I think the code on the right is okay.

http://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:10012: int32_t c = this->CharAt(i);
This also looks like noise.

http://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newco...
runtime/vm/object.cc:10016: if (c > Utf16::kMaxCodeUnit) i++;
I am confused about this line.  A comment would be helpful.

[erikcorry, 2012-11-19 12:40:41]

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.dart
File runtime/lib/string_base.dart (right):

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:18: static String createFromUtf16(List<int>
codeUnits) {
On 2012/11/16 22:32:04, siva wrote:
> The comment
> "Create the .... for specified UTF-16 code units" would
> still be valid right ?

Done.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:315: return newResult;
On 2012/11/16 22:32:04, siva wrote:
> This piece of code is repeated 3 times in this CL, why not abstract it out.

One copy removed, last three lines changed to a getRange call.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:389: class _TwoByteStringBase extends _StringBase {
On 2012/11/16 22:32:04, siva wrote:
> Maybe add a TODO here to get rid of this class and use
> mixins instead once they are implemented in dart.

I don't see why we would want to do that.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:429: List<int> get charCodes {
On 2012/11/16 22:32:04, siva wrote:
> TODO, fix this to use the new code point iterator when it is available?

Done.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:442: if (i == j) return result;
On 2012/11/16 22:32:04, siva wrote:
> I find the (i == j) condition here a little unreadable,
> why not introduce a boolean variable non_bmp_char_seen
> and test for that.
> 
> The same comment also holds for the splitChars case.

Done.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:450: }
On 2012/11/16 22:32:04, siva wrote:
> This code here and in splitChars above seem to be identical,
> it might make sense to abstract it out (maybe a static
> method that is passed in a string object).

Last three lines fixed to use getRange, but splitChars is not really identical
since it returns a list of strings, whereas this one returns a list of ints.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:462: // TODO(erikcorry): This is going away.
On 2012/11/16 22:32:04, siva wrote:
> (why not remove it in this CL itself)?

Done.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_base.d...
runtime/lib/string_base.dart:507: // TODO(erikcorry): This is going away.
On 2012/11/16 22:32:04, siva wrote:
> Ditto comment.

Done.

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_patch....
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/11368138/diff/11002/runtime/lib/string_patch....
runtime/lib/string_patch.dart:7: return _StringBase.createFromUtf16(codeUnits);
On 2012/11/16 22:32:04, siva wrote:
> I think we have consistently used the same name when
> patching in functions in order to avoid confusion, why
> not call it String.createFromUtf16

I'm defering to your opinion on the implementation name, but the correct API
name in the core libraries has not yet been determined.

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:9895: return OneByteString::CharAt(*this, index);
On 2012/11/17 02:25:27, cshapiro wrote:
> Why was this changed?  I think the code on the right is okay.

Reverted.

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10012: int32_t c = this->CharAt(i);
On 2012/11/17 02:25:27, cshapiro wrote:
> This also looks like noise.

This function was an attempt to compare a UTF-32 string in a C array with a Dart
UTF-16 string.  It didn't work, and I have removed it and fixed the place it was
used (the scanner) to do its UTF-16 expansion itself.

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10016: if (c > Utf16::kMaxCodeUnit) i++;
On 2012/11/17 02:25:27, cshapiro wrote:
> I am confused about this line.  A comment would be helpful.

Ditto

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10017: }
On 2012/11/16 22:32:04, siva wrote:
> I don't quite get this implementation.
Ditto.

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.cc#newc...
runtime/vm/object.cc:10803: ASSERT(ch <= Utf16::kMaxCodePoint);
On 2012/11/16 22:32:04, siva wrote:
> We assert here that ch <= Utf16::kMaxCodePoint
> but we have code below that deals with the case when
> ch > Utf16::kMaxCodePoint
> 
> The ASSERT should be removed?

No, the assert checks against kMaxCodePoint (0x10ffff) and the 'if' checks
agains kMaxCodeUnit (0xffff).

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.h#newco...
runtime/vm/object.h:3724: // always use this if you are looking for ASCII.  If
you need to support
On 2012/11/16 22:32:04, siva wrote:
> Should we say ASCII or Latin1 here?

Now says both.

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.h#newco...
runtime/vm/object.h:3950: int out_length,
On 2012/11/16 22:32:04, siva wrote:
> we always use intptr_t for length values in the VM

Done.

https://codereview.chromium.org/11368138/diff/11002/runtime/vm/object.h#newco...
runtime/vm/object.h:4034: int out_length,
On 2012/11/16 22:32:04, siva wrote:
> Ditto.

Done.

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart
File sdk/lib/core/string.dart (right):

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:30: factory String.fromCharCodes(List<int> charCodes) {
On 2012/11/16 22:32:04, siva wrote:
> why not call this createFromUtf32

The exact name of this is to be determined by floitsch and I already have his
LGTM.  I'm sure he'll rename it when he has decided on a name.

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:69: external factory String.fromCodeUnits(List<int>
codeUnits);
On 2012/11/16 22:32:04, siva wrote:
> Should be called  createFromUtf16 to avoid confusion
> you have already renamed the VM method.

As above, up to floitsch.

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:192: List<int> get charCodes {
On 2012/11/16 22:32:04, siva wrote:
> TODO use codepoint iterator instead of charCodeAt here?

This method was a dupe.  Removed.

https://codereview.chromium.org/11368138/diff/11002/sdk/lib/core/string.dart#...
sdk/lib/core/string.dart:211: return newResult;
On 2012/11/16 22:32:04, siva wrote:
> This piece of code was repeated earlier too, maybe abstract
> it out.

Ditto.

[Søren Gjesse, 2012-11-19 14:18:36]

dbc

http://codereview.chromium.org/11368138/diff/16046/runtime/vm/unicode.h
File runtime/vm/unicode.h (right):

http://codereview.chromium.org/11368138/diff/16046/runtime/vm/unicode.h#newco...
runtime/vm/unicode.h:32: static int32_t CodePointFromCodeUnits(int32_t lead,
int32_t trail) {
Assert that lead is lead surrogare and that train is a trail surrogate.

[cshapiro, 2012-11-20 00:22:02]

Hi Erik,

I have added a code point iterator to the VM that supports its string types. 
That change resolves many of the same issues you are trying to address in the VM
but in a different way.  This changes the scope of the VM part of this change.

Thanks,

Carl

[siva, 2012-11-20 00:48:00]

When you sync up to TOT you may find that a number of the
VM changes are now fixed with the Iterator change that
Carl submitted.

Your changes LGTM once you have resolved to TOT.

https://codereview.chromium.org/11368138/diff/16046/runtime/vm/scanner.cc
File runtime/vm/scanner.cc (right):

https://codereview.chromium.org/11368138/diff/16046/runtime/vm/scanner.cc#new...
runtime/vm/scanner.cc:563: Symbols::New(string_chars.data(),
string_chars.length()));
Is the change above necessary considering that this
method calls the uint32 version of String::New which accounts
for supplementary characters.