String encoding utility methods and tests for Unicode, UTF-8, -16 and -32.

String encoding utility methods and tests for Unicode, UTF-8, -16 and -32.


BUG=
TEST=

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

[dcarlson, 2012-01-31 14:15:54]

Initial stab at (sharable) Unicode/UTF library.

[ahe, 2012-01-31 14:46:52]

Initial comments. Don't wait for my approval, it'll take me some time to get
through UTF-16.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Unicode.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:13: if (is16BitCodeUnit()) {
I think you're working around bug in frog or dartc here. I suggest that you add
a TODO and file a bug to track this.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/Utf8Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/Utf8Tests.dart:49: encodeAsUtf8("Quizdeltagerne
spiste jordbær med fløde, mens " +
Remove comma.

[dcarlson, 2012-01-31 15:07:34]

Thanks.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Unicode.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:13: if (is16BitCodeUnit()) {
On 2012/01/31 14:46:53, ahe wrote:
> I think you're working around bug in frog or dartc here. I suggest that you
add
> a TODO and file a bug to track this.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/Utf8Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/Utf8Tests.dart:49: encodeAsUtf8("Quizdeltagerne
spiste jordbær med fløde, mens " +
On 2012/01/31 14:46:53, ahe wrote:
> Remove comma.

Done. Thanks, my Danish is rusty ;).

[jat, 2012-01-31 15:19:21]

Generally looks good, with thorough tests.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Unicode.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
Copyright date.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/UnicodeCore.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:8: * Test for presence of bug related to
the use of UTF-16 code units for
I'm not sure I would call it a bug -- it simply isn't practical to implement
UCS32 in JS, as you wouldn't be able to make much use of native string support
in JS.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:14: _test16BitCodeUnit = (new
String.fromCharCodes([0x1D11E])) ==
I would have expected this to result in a 2-character string in JS (ie, the
surrogate pair corresponding to the non-BMP character).  If it isn't, then this
is definitely a bug (in fromCharCodes and perhaps elsewhere) as non-BMP
characters simply aren't supported at all when compiled to JS -- in JS if I give
it a surrogate pair the non-BMP character is properly displayed.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:30: [int start = 0, int length = -1]) {
Seems like null is a better "not-supplied" default than -1 to avoid confusion,
since in many APIs a negative length means counting from the end.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:42: codeUnitsBuffer.add(0xdc00 + (base &
0x3ff));
These ranges should be in constants rather than repeating them.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:63: if(end > 0 && utf16CodeUnits[0] ==
0xfeff) {
If you are checking for BOM, do you want to also check for U+FFFE and reverse
the bytes below?

I note you don't generate BOM above.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf16.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:55: List<int> encoding = <int>[0xfe, 0xff];
Is this supposed to have BOM?  I thought UTF16-BE and UTF16-LE never included
BOM since you know the byte order by external agreement?

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf32.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf32.dart:37: List<int> encoding = <int>[0, 0, 0xfe,
0xff];
Same comment as UTF16-BE/LE here.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf8_impl.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:35: int i = start;
Consider counting the size of the output buffer first - it might be better than
dynamically growing it.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:44: encoded.insertRange(encoded.length, 2,
99);
What is the 99 for here?

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:59: } else if (value <= 0x7fffffff) {
You are inconsistent -- elsewhere, you restrict codepoints to the valid range of
0x10FFFF (which only needs 4 bytes to encode in UTF8) -- you should either
decide to only support valid Unicode codepoints everywhere or allow 31-bit
codepoints everywhere.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:69: * UTF-8 bytes are utf8 code units, so
we can go directly from
This comment is confusing to me, can you explain what you mean?

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:111: // equiv to (value << 6) | (nextValue
& 0x3f)
Why not use the shift?  Comment should at least explain why.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/DUnit.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/DUnit.dart:6: #library("DUnit");
Should this be part of this change?  It looks like a stand-alone unit test
facility.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/RunTests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/RunTests.dart:20: void registerTests(TestSuite
suite) {
Is there really any value in separating this out of main?

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/UnicodeCoreTests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/UnicodeCoreTests.dart:19:
register("testCodepointsToUtf16CodeUnits", testCodepointsToUtf16CodeUnits,
suite);
Line length.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/Utf8Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/Utf8Tests.dart:173: Expect.listEquals([0xfffd],
All these 0xfffd references should refer to the constant.

https://chromiumcodereview.appspot.com/9233041/diff/2014/utils/string_encodin...
File utils/string_encoding/Utf8_impl.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/2014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:106: }
Do you want to reject over-long encodings?  They can be used to subvert some
checks (ie, encoding an ASCII < as a multi-byte sequence to avoid simplistic
HTML sanitizers).

[Dan Rice, 2012-01-31 15:59:54]

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Unicode.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:13: if (is16BitCodeUnit()) {
+1

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:26: return (new String.fromCharCodes(
No need for parens around return value

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:29: return (new
String.fromCharCodes(codepoints));
Ditto

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/UnicodeCore.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:8: * Test for presence of bug related to
the use of UTF-16 code units for
Since strings are immutable, you can at least do all the work up front at
construction time, and in the case where there are no surrogate pairs the
constructed instance can rely on natives.  This seems like it might be worth
prototyping and benchmarking.

On 2012/01/31 15:19:22, jat wrote:
> I'm not sure I would call it a bug -- it simply isn't practical to implement
> UCS32 in JS, as you wouldn't be able to make much use of native string support
> in JS.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:14: _test16BitCodeUnit = (new
String.fromCharCodes([0x1D11E])) ==
On 2012/01/31 15:19:22, jat wrote:
> I would have expected this to result in a 2-character string in JS (ie, the
> surrogate pair corresponding to the non-BMP character).  If it isn't, then
this
> is definitely a bug (in fromCharCodes and perhaps elsewhere) as non-BMP
> characters simply aren't supported at all when compiled to JS -- in JS if I
give
> it a surrogate pair the non-BMP character is properly displayed.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:30: [int start = 0, int length = -1]) {
Maybe use length = null as the sentinel?  The -1 makes me expect some kind of
general behavior defined for negative values.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:37: if ((value >= 0 && value <0xd800) ||
(value >= 0xe000 && value <0x10000)) {
space after '<'

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:39: } else if (value >= 0x10000 && value
< 0x110000){
Space before '{'

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:54: [int start = 0, int length = - 1]) {
length sentinel

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:63: if(end > 0 && utf16CodeUnits[0] ==
0xfeff) {
space before '('

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:74: value = (value - 0xd800) * 0x400;
can use '<< 10' instead of '* 0x400'

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:80: if (nextValue >= 0xd800 && nextValue
< 0xdc00) i--;
better not to put the 'then' clause on same line

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf16.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:15: return new String.fromCharCodes(codeUnits);
Comment that this account for platform-specific String behavior

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:27: return new String.fromCharCodes(codeUnits);
Ditto

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:55: List<int> encoding = <int>[0xfe, 0xff];
On 2012/01/31 15:19:22, jat wrote:
> Is this supposed to have BOM?  I thought UTF16-BE and UTF16-LE never included
> BOM since you know the byte order by external agreement?

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:90: [int start = 0, int length = - 1]) {
length = null

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:100: codeUnits.add((hi * 256) + lo);
use shift?

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:110: [int start = 0, int length = - 1]) {
length = null

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:136: if((start + 1 < end) &&
utf16EncodedBytes[start] == 0xfe &&
use 'start + 1 < end' or 'start + 2 <= end' consistently

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf32.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf32.dart:35: List<int> encodeAsUtf32be(String str) {
, [bool writeBom = false]

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf8_impl.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:29: List<int> _codepointsToUtf8(List<int>
codepoints,
length null

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:115: if (nextValue >= 0xc0) i--;
line break after )

[Dan Rice, 2012-01-31 16:01:10]

https://chromiumcodereview.appspot.com/9233041/diff/2014/utils/tests/string_e...
File utils/tests/string_encoding/Utf8Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/2014/utils/tests/string_e...
utils/tests/string_encoding/Utf8Tests.dart:410: Expect.stringEquals("? דג סקרן
שט בים מאוכזב ולפתע מצא לו חברה איך הקלי",
הקליטה

[dcarlson, 2012-01-31 22:11:38]

Thanks for all the feedback. I think most concerns are addressed. Please holler
if I got anything wrong.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Unicode.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:1: // Copyright (c) 2011, the Dart project
authors.  Please see the AUTHORS file
On 2012/01/31 15:19:22, jat wrote:
> Copyright date.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:26: return (new String.fromCharCodes(
On 2012/01/31 15:59:55, Dan Rice wrote:
> No need for parens around return value

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Unicode.dart:29: return (new
String.fromCharCodes(codepoints));
On 2012/01/31 15:59:55, Dan Rice wrote:
> Ditto

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/UnicodeCore.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:8: * Test for presence of bug related to
the use of UTF-16 code units for
On 2012/01/31 15:59:55, Dan Rice wrote:
> Since strings are immutable, you can at least do all the work up front at
> construction time, and in the case where there are no surrogate pairs the
> constructed instance can rely on natives.  This seems like it might be worth
> prototyping and benchmarking.
> 
> On 2012/01/31 15:19:22, jat wrote:
> > I'm not sure I would call it a bug -- it simply isn't practical to implement
> > UCS32 in JS, as you wouldn't be able to make much use of native string
support
> > in JS.
> 

The calculation is once, then cached.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:14: _test16BitCodeUnit = (new
String.fromCharCodes([0x1D11E])) ==
On 2012/01/31 15:19:22, jat wrote:
> I would have expected this to result in a 2-character string in JS (ie, the
> surrogate pair corresponding to the non-BMP character).  If it isn't, then
this
> is definitely a bug (in fromCharCodes and perhaps elsewhere) as non-BMP
> characters simply aren't supported at all when compiled to JS -- in JS if I
give
> it a surrogate pair the non-BMP character is properly displayed.

:)

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:30: [int start = 0, int length = -1]) {
On 2012/01/31 15:59:55, Dan Rice wrote:
> Maybe use length = null as the sentinel?  The -1 makes me expect some kind of
> general behavior defined for negative values.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:37: if ((value >= 0 && value <0xd800) ||
(value >= 0xe000 && value <0x10000)) {
On 2012/01/31 15:59:55, Dan Rice wrote:
> space after '<'

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:39: } else if (value >= 0x10000 && value
< 0x110000){
On 2012/01/31 15:59:55, Dan Rice wrote:
> Space before '{'

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:42: codeUnitsBuffer.add(0xdc00 + (base &
0x3ff));
On 2012/01/31 15:19:22, jat wrote:
> These ranges should be in constants rather than repeating them.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:54: [int start = 0, int length = - 1]) {
On 2012/01/31 15:59:55, Dan Rice wrote:
> length sentinel

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:63: if(end > 0 && utf16CodeUnits[0] ==
0xfeff) {
On 2012/01/31 15:19:22, jat wrote:
> If you are checking for BOM, do you want to also check for U+FFFE and reverse
> the bytes below?
> 
> I note you don't generate BOM above.

Not here. Only do this when encoding code units to bytes.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:63: if(end > 0 && utf16CodeUnits[0] ==
0xfeff) {
On 2012/01/31 15:19:22, jat wrote:
> If you are checking for BOM, do you want to also check for U+FFFE and reverse
> the bytes below?
> 
> I note you don't generate BOM above.

Not when going from codepoint to code unit. The BOM is introduced only when we
go form code unit to encoded bytes (where we would know 1) the byte order and 2)
where we could decide whether we want to include the BOM).

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:74: value = (value - 0xd800) * 0x400;
On 2012/01/31 15:59:55, Dan Rice wrote:
> can use '<< 10' instead of '* 0x400'

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:80: if (nextValue >= 0xd800 && nextValue
< 0xdc00) i--;
On 2012/01/31 15:59:55, Dan Rice wrote:
> better not to put the 'then' clause on same line

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf16.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:15: return new String.fromCharCodes(codeUnits);
On 2012/01/31 15:59:55, Dan Rice wrote:
> Comment that this account for platform-specific String behavior

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:27: return new String.fromCharCodes(codeUnits);
On 2012/01/31 15:59:55, Dan Rice wrote:
> Ditto

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:55: List<int> encoding = <int>[0xfe, 0xff];
On 2012/01/31 15:19:22, jat wrote:
> Is this supposed to have BOM?  I thought UTF16-BE and UTF16-LE never included
> BOM since you know the byte order by external agreement?

It is the BOM --  not required, but currently stripping if provided (e.g. it has
a BOM).

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:90: [int start = 0, int length = - 1]) {
On 2012/01/31 15:59:55, Dan Rice wrote:
> length = null

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:100: codeUnits.add((hi * 256) + lo);
On 2012/01/31 15:59:55, Dan Rice wrote:
> use shift?

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:110: [int start = 0, int length = - 1]) {
On 2012/01/31 15:59:55, Dan Rice wrote:
> length = null

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:136: if((start + 1 < end) &&
utf16EncodedBytes[start] == 0xfe &&
On 2012/01/31 15:59:55, Dan Rice wrote:
> use 'start + 1 < end' or 'start + 2 <= end' consistently

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf32.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf32.dart:35: List<int> encodeAsUtf32be(String str) {
On 2012/01/31 15:59:55, Dan Rice wrote:
> , [bool writeBom = false]

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf32.dart:37: List<int> encoding = <int>[0, 0, 0xfe,
0xff];
On 2012/01/31 15:19:22, jat wrote:
> Same comment as UTF16-BE/LE here.

OK

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf8_impl.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:29: List<int> _codepointsToUtf8(List<int>
codepoints,
On 2012/01/31 15:59:55, Dan Rice wrote:
> length null

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:35: int i = start;
On 2012/01/31 15:19:22, jat wrote:
> Consider counting the size of the output buffer first - it might be better
than
> dynamically growing it.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:44: encoded.insertRange(encoded.length, 2,
99);
On 2012/01/31 15:19:22, jat wrote:
> What is the 99 for here?

It slipped through. Ooops.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:59: } else if (value <= 0x7fffffff) {
On 2012/01/31 15:19:22, jat wrote:
> You are inconsistent -- elsewhere, you restrict codepoints to the valid range
of
> 0x10FFFF (which only needs 4 bytes to encode in UTF8) -- you should either
> decide to only support valid Unicode codepoints everywhere or allow 31-bit
> codepoints everywhere.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:69: * UTF-8 bytes are utf8 code units, so
we can go directly from
On 2012/01/31 15:19:22, jat wrote:
> This comment is confusing to me, can you explain what you mean?

Try the updated text.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:111: // equiv to (value << 6) | (nextValue
& 0x3f)
On 2012/01/31 15:19:22, jat wrote:
> Why not use the shift?  Comment should at least explain why.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:115: if (nextValue >= 0xc0) i--;
On 2012/01/31 15:59:55, Dan Rice wrote:
> line break after )

Done. Was trying to stick with the style guide
(http://www.dartlang.org/articles/style-guide/)

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/DUnit.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/DUnit.dart:6: #library("DUnit");
On 2012/01/31 15:19:22, jat wrote:
> Should this be part of this change?  It looks like a stand-alone unit test
> facility.

True. I needed it to include my tests -- trying to clarify best path here...

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/RunTests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/RunTests.dart:20: void registerTests(TestSuite
suite) {
On 2012/01/31 15:19:22, jat wrote:
> Is there really any value in separating this out of main?

Perhaps -- the intention was to easily group tests across classes.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/UnicodeCoreTests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/UnicodeCoreTests.dart:19:
register("testCodepointsToUtf16CodeUnits", testCodepointsToUtf16CodeUnits,
suite);
On 2012/01/31 15:19:22, jat wrote:
> Line length.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
File utils/tests/string_encoding/Utf8Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/tests/string_e...
utils/tests/string_encoding/Utf8Tests.dart:173: Expect.listEquals([0xfffd],
On 2012/01/31 15:19:22, jat wrote:
> All these 0xfffd references should refer to the constant.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/2014/utils/string_encodin...
File utils/string_encoding/Utf8_impl.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/2014/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:106: }
On 2012/01/31 15:19:22, jat wrote:
> Do you want to reject over-long encodings?  They can be used to subvert some
> checks (ie, encoding an ASCII < as a multi-byte sequence to avoid simplistic
> HTML sanitizers).

:) I do. See the tests for checks on overlong encodings as confirmation. The
filtering takes place below.

https://chromiumcodereview.appspot.com/9233041/diff/2014/utils/tests/string_e...
File utils/tests/string_encoding/Utf8Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/2014/utils/tests/string_e...
utils/tests/string_encoding/Utf8Tests.dart:410: Expect.stringEquals("? דג סקרן
שט בים מאוכזב ולפתע מצא לו חברה איך הקלי",
On 2012/01/31 16:01:10, Dan Rice wrote:
> הקליטה

Done.

[jat, 2012-01-31 22:46:12]

LGTM

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf16.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:55: List<int> encoding = <int>[0xfe, 0xff];
On 2012/01/31 22:11:38, dcarlson wrote:
> It is the BOM --  not required, but currently stripping if provided (e.g. it
has
> a BOM).

My recollection is some libraries treat the presence of BOM in the case of
UTF16-LE/BE as an error, since you are supposed to already know what it is --
otherwise, there is no value of having UTF16-LE/BE, just use UTF16 if you are
going to have BOM anyway.

I am assuming this isn't an issue for what you are interoperating with, so if
you want to leave it the way it is I am ok with that.

[Søren Gjesse, 2012-02-01 11:25:25]

ltgm

Most of my comments are suggestions that you can consider.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
File utils/string_encoding/Unicode.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Unicode.dart:5: #library("Unicode");
I think our current naming scheme for libraries is lowercase only.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Unicode.dart:6: #import("UnicodeCore.dart");
Same for file names. All Dart files in e.g. corelib/src uses all lowercase and
underscores.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
File utils/string_encoding/UnicodeCore.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:45: * Encode code points as UTF16 code
units.
Please be consistent with UTF16/utf16 in comments. Actually the "official" name
seems to be UTF-16.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:86: [int start = 0, int length = null]) {
Normally we don't split argument lists like this. Won't

List<int> utf16CodeUnitsToCodepoints(
    List<int> utf16CodeUnits, [int start = 0, int length = null]) {

fit?

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
File utils/string_encoding/Utf16.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf16.dart:5: #library("UTF16");
Maybe utf16?

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
File utils/string_encoding/Utf8_impl.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:35: String decodeFromUtf8(List<int> bytes)
=>
Should we perhaps have an optional offset and length field here. We do in a lot
of places where byte Lists are passed. In some cases you might like to decode
parts of a network buffer received.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:35: String decodeFromUtf8(List<int> bytes)
=>
Should there be optional arguments for:

1. Specifying an alternative "replacement character"
2. Requesting an Exception instead of decoding to the "replacement character".

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:36:
codepointsToString(_utf8ToCodepoints(bytes, 0, bytes.length));
Consider having both _utf8ToCodepoints and _utf8ToString where _utf8ToString
fills characters directly into a StringBuffer and returns a string. That avoids
allocating the code points array.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:114: [int start = 0, int length = null]) {
I think corelib normally uses the name offset instead of start.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:127: List<int> codepointBuffer = <int>[];
Have you thought about having an initial scan for calculating the number of
decoded codepoints first (like you calculate encodedLength above)?

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:147: additionalBytes = 3;
Do we ever want to decode more than 4 bytes. There are no defined codepoints
that require more AFAIK.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
File utils/tests/string_encoding/UnicodeCoreTests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
utils/tests/string_encoding/UnicodeCoreTests.dart:29: // boundary conditions
How about having a shared list of the "sane" test data:

... = [[0x0], "0"],
       [0xd800, 0xdc00], "10000",
       ...];

And reuse then in both tests.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
File utils/tests/string_encoding/Utf16Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
utils/tests/string_encoding/Utf16Tests.dart:22: void testEncodeToUtf16() {
Same here, have duplicated test data in a shared list.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
File utils/tests/string_encoding/Utf32Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
utils/tests/string_encoding/Utf32Tests.dart:22: void testEncodeToUtf32() {
Again, share duplicate test data.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
File utils/tests/string_encoding/Utf8Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
utils/tests/string_encoding/Utf8Tests.dart:192:
matchingReplacementChars.addAll([UNICODE_REPLACEMENT_CHARACTER_CODEPOINT]);
Long line here and some more places below.

[dcarlson, 2012-02-01 22:18:46]

I think we are up to date here.

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
File utils/string_encoding/Utf16.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/1014/utils/string_encodin...
utils/string_encoding/Utf16.dart:55: List<int> encoding = <int>[0xfe, 0xff];
On 2012/01/31 22:46:12, jat wrote:
> On 2012/01/31 22:11:38, dcarlson wrote:
> > It is the BOM --  not required, but currently stripping if provided (e.g. it
> has
> > a BOM).
> 
> My recollection is some libraries treat the presence of BOM in the case of
> UTF16-LE/BE as an error, since you are supposed to already know what it is --
> otherwise, there is no value of having UTF16-LE/BE, just use UTF16 if you are
> going to have BOM anyway.
> 
> I am assuming this isn't an issue for what you are interoperating with, so if
> you want to leave it the way it is I am ok with that.

So, I'm switching things up a little. I want to provide flexibility to the user,
because I don't necessarily know about the context (e.g. the protocol). I don't
throw an exception, but include a parameter that can specify whether to strip
the BOM.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
File utils/string_encoding/Unicode.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Unicode.dart:5: #library("Unicode");
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> I think our current naming scheme for libraries is lowercase only.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Unicode.dart:6: #import("UnicodeCore.dart");
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Same for file names. All Dart files in e.g. corelib/src uses all lowercase and
> underscores.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
File utils/string_encoding/UnicodeCore.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:45: * Encode code points as UTF16 code
units.
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Please be consistent with UTF16/utf16 in comments. Actually the "official"
name
> seems to be UTF-16.

Done. I went ahead and made sure all the variants match the spec.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/UnicodeCore.dart:86: [int start = 0, int length = null]) {
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Normally we don't split argument lists like this. Won't
> 
> List<int> utf16CodeUnitsToCodepoints(
>     List<int> utf16CodeUnits, [int start = 0, int length = null]) {
> 
> fit?
Not always (other lines), but done with a simple line wrap.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
File utils/string_encoding/Utf16.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf16.dart:5: #library("UTF16");
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Maybe utf16?

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
File utils/string_encoding/Utf8_impl.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:35: String decodeFromUtf8(List<int> bytes)
=>
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Should we perhaps have an optional offset and length field here. We do in a
lot
> of places where byte Lists are passed. In some cases you might like to decode
> parts of a network buffer received.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:35: String decodeFromUtf8(List<int> bytes)
=>
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Should we perhaps have an optional offset and length field here. We do in a
lot
> of places where byte Lists are passed. In some cases you might like to decode
> parts of a network buffer received.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:36:
codepointsToString(_utf8ToCodepoints(bytes, 0, bytes.length));
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Consider having both _utf8ToCodepoints and _utf8ToString where _utf8ToString
> fills characters directly into a StringBuffer and returns a string. That
avoids
> allocating the code points array.

I don't think this buys us much. The StringBuffer is a List (of strings, but we
are adding a character at a time, so no real win). If we can figure the length
ahead of time, we could allocate a ByteArray to perhaps reduce the # of objects
allocated at any one time.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:114: [int start = 0, int length = null]) {
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> I think corelib normally uses the name offset instead of start.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:127: List<int> codepointBuffer = <int>[];
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Have you thought about having an initial scan for calculating the number of
> decoded codepoints first (like you calculate encodedLength above)?

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/string_encodin...
utils/string_encoding/Utf8_impl.dart:147: additionalBytes = 3;
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Do we ever want to decode more than 4 bytes. There are no defined codepoints
> that require more AFAIK.

No valid codepoints, but the spec defines decodings out to 6 bytes -- I wanted
to keep these because although will treat it as an invalid character, we don't
want to get out of sync.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
File utils/tests/string_encoding/UnicodeCoreTests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
utils/tests/string_encoding/UnicodeCoreTests.dart:29: // boundary conditions
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> How about having a shared list of the "sane" test data:
> 
> ... = [[0x0], "0"],
>        [0xd800, 0xdc00], "10000",
>        ...];
> 
> And reuse then in both tests.

Thanks for the suggestion -- I think this is clearer for the number of
datapoints included here.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
File utils/tests/string_encoding/Utf16Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
utils/tests/string_encoding/Utf16Tests.dart:22: void testEncodeToUtf16() {
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Same here, have duplicated test data in a shared list.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
File utils/tests/string_encoding/Utf32Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
utils/tests/string_encoding/Utf32Tests.dart:22: void testEncodeToUtf32() {
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Again, share duplicate test data.

Done.

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
File utils/tests/string_encoding/Utf8Tests.dart (right):

https://chromiumcodereview.appspot.com/9233041/diff/9001/utils/tests/string_e...
utils/tests/string_encoding/Utf8Tests.dart:192:
matchingReplacementChars.addAll([UNICODE_REPLACEMENT_CHARACTER_CODEPOINT]);
On 2012/02/01 11:25:25, Søren Gjesse wrote:
> Long line here and some more places below.

Done.