Replace old utf8 decoder with new one.

Replace old utf8 decoder with new one.

R=lrn@google.com

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

[floitsch, 2013-07-15 13:48:54]

Added comments.

[Lasse Reichstein Nielsen, 2013-07-16 12:23:02]

lgtm

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dart
File sdk/lib/codec/encoding.dart (right):

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:33: * the [decoder] replace invalid (or
unterminated) character
character sequences -> octet sequences

Actually, you can't generalize between the encoder and the decoder.
The encoder cannot fail since it takes a String as input, which always contains
"UTF-16 + unpaired surrogates". 
Is an unpaired surrogate an error for the encoder? (I see below that the encoder
doesn't use 'allowMalformed' at all, so just don't mention it here).

The decoder can see invalid octet sequences, and it can see non-canonical
encodings. I assume both are errors, and will either be replaced or throw.

What about the sequence [0x80, 0x80, 0x80]. Is that one error or three?

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:34: * sequences with the Unicode Replacement
character `0xFFFD` (�). Otherwise
'0xFFFD' -> U+FFFD

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:44: * If [allowMalformed] is `true` replaces invalid
(or unterminated) character
... is `true`, the decoder replaces ...

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:45: * sequences with the Unicode Replacement
character `0xFFFD` (�). Otherwise
U+FFFD

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:46: * throws a [FormatException].
Otherwise it throws ...

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:48: * If [allowMalformed] is not given, defaults to
the `allowMalformed` that
'it' after comma.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart
File sdk/lib/convert/utf.dart (right):

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:33: * sequences with the Unicode Replacement character
`0xFFFD` (�). Otherwise
U+FFFD

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:79: * Decodes utf-8.
UTF-8.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:83: // TODO(floitsch): do we want to make this class
public?
Sure, why not?

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:167: _throwIfNecessary("Bad UTF-8 encoding
0x${unit.toRadixString(16)}");
Seems inefficient to create the string and not use it.
I'd rather inline the _throwIfNecessary, or move the string interpolation into
_throwIfNecessary (e.g., only pass the constant string before "0x", and the bad
value, as arguments, and build the rest if necessary)

https://codereview.chromium.org/19187002/diff/5001/tests/lib/convert/utf82_te...
File tests/lib/convert/utf82_test.dart (right):

https://codereview.chromium.org/19187002/diff/5001/tests/lib/convert/utf82_te...
tests/lib/convert/utf82_test.dart:36: [ 0xF0, 0x82, 0x82, 0xAC ]];
Test with "[0x80, 0x80]" too.
And decide what happens. I'd make it one invalid sequence (which is what it
looks like if you are moving backwards - it's an uninitiated encoding, not
unterminated).

How about overlong encodings? (e.g., [0xC0, 0x80])?

And how about encoded surrogates?

[floitsch, 2013-07-16 14:25:23]

PTAL.
I added removal of leading BOMs.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dart
File sdk/lib/codec/encoding.dart (right):

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:33: * the [decoder] replace invalid (or
unterminated) character
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> character sequences -> octet sequences
> 
> Actually, you can't generalize between the encoder and the decoder.
> The encoder cannot fail since it takes a String as input, which always
contains
> "UTF-16 + unpaired surrogates". 
> Is an unpaired surrogate an error for the encoder? (I see below that the
encoder
> doesn't use 'allowMalformed' at all, so just don't mention it here).
> 
> The decoder can see invalid octet sequences, and it can see non-canonical
> encodings. I assume both are errors, and will either be replaced or throw.
> 
> What about the sequence [0x80, 0x80, 0x80]. Is that one error or three?

As discussed: the sentence before says that it only applies to the decode part.

Will add tests for 0x80.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:34: * sequences with the Unicode Replacement
character `0xFFFD` (�). Otherwise
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> '0xFFFD' -> U+FFFD

Done.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:44: * If [allowMalformed] is `true` replaces invalid
(or unterminated) character
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> ... is `true`, the decoder replaces ...

Done.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:45: * sequences with the Unicode Replacement
character `0xFFFD` (�). Otherwise
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> U+FFFD

Done.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:46: * throws a [FormatException].
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> Otherwise it throws ...

Done.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/codec/encoding.dar...
sdk/lib/codec/encoding.dart:48: * If [allowMalformed] is not given, defaults to
the `allowMalformed` that
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> 'it' after comma.

Done.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart
File sdk/lib/convert/utf.dart (right):

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:33: * sequences with the Unicode Replacement character
`0xFFFD` (�). Otherwise
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> U+FFFD

Done.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:79: * Decodes utf-8.
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> UTF-8.

Done.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:83: // TODO(floitsch): do we want to make this class
public?
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> Sure, why not?

Later. But rephrased TODO.

https://codereview.chromium.org/19187002/diff/5001/sdk/lib/convert/utf.dart#n...
sdk/lib/convert/utf.dart:167: _throwIfNecessary("Bad UTF-8 encoding
0x${unit.toRadixString(16)}");
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> Seems inefficient to create the string and not use it.
> I'd rather inline the _throwIfNecessary, or move the string interpolation into
> _throwIfNecessary (e.g., only pass the constant string before "0x", and the
bad
> value, as arguments, and build the rest if necessary)

inlined.

https://codereview.chromium.org/19187002/diff/5001/tests/lib/convert/utf82_te...
File tests/lib/convert/utf82_test.dart (right):

https://codereview.chromium.org/19187002/diff/5001/tests/lib/convert/utf82_te...
tests/lib/convert/utf82_test.dart:36: [ 0xF0, 0x82, 0x82, 0xAC ]];
On 2013/07/16 12:23:03, Lasse Reichstein Nielsen wrote:
> Test with "[0x80, 0x80]" too.
> And decide what happens. I'd make it one invalid sequence (which is what it
> looks like if you are moving backwards - it's an uninitiated encoding, not
> unterminated).
I prefer making each one a separate error.
Otherwise it is not clear what 0x80, 0x80, 0x80 should be.
There could be 1, 2, or 3 missing bytes.
> 
> How about overlong encodings? (e.g., [0xC0, 0x80])?
done.
> 
> And how about encoded surrogates?
I would prefer not to check those. But I can if you prefer.

[Lasse Reichstein Nielsen, 2013-07-17 09:14:50]

lgtm

https://codereview.chromium.org/19187002/diff/5001/tests/lib/convert/utf82_te...
File tests/lib/convert/utf82_test.dart (right):

https://codereview.chromium.org/19187002/diff/5001/tests/lib/convert/utf82_te...
tests/lib/convert/utf82_test.dart:36: [ 0xF0, 0x82, 0x82, 0xAC ]];
The point is that UTF-8 can be traversed both forwards and backwards.
If you traverse it backwards, you will pass all three 0x80's before realizing
that something is missing. At that point, it would make sense to make them one
error. I'd do the same when moving forward.

[floitsch, 2013-07-17 11:35:21]

Committed patchset #4 manually as r25089 (presubmit successful).