Fix length computation in Dart_StringToUTF8.

Fix length computation in Dart_StringToUTF8.
Committed: https://code.google.com/p/dart/source/detail?r=14588

[Bill Hesse, 2012-11-01 20:38:01]

http://codereview.chromium.org/11275107/diff/1/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/11275107/diff/1/bin/builtin_natives.cc#newcode112
bin/builtin_natives.cc:112: length = (length * sizeof(uint16_t));
This is not enough.  Some 16-bit code points are encoded in 3 bytes in UTF8.

I am assuming that StringLength is reporting a surrogate pair as two characters,
and the UTF8 encoder is actually encoding CESU-8:

Main article: CESU-8
Many programs added UTF-8 conversions for UCS-2 data and did not alter this
UTF-8 conversion when UCS-2 was replaced with the surrogate-pair using UTF-16.
In such programs each half of a UTF-16 surrogate pair is encoded as its own
3-byte UTF-8 encoding, resulting in 6-byte sequences rather than 4 bytes for
characters outside the Basic Multilingual Plane. Oracle and MySQL databases use
this, as well as Java and Tcl as described below, and probably many Windows
programs where the programmers were unaware of the complexities of UTF-16.
Although this non-optimal encoding is generally not deliberate, a supposed
benefit is that it preserves UTF-16 binary sorting order when CESU-8 is binary
sorted.

http://codereview.chromium.org/11275107/diff/1/bin/builtin_natives.cc#newcode116
bin/builtin_natives.cc:116: result = Dart_StringToUTF8(str, chars, &length);
I don't see what the problem is with using Dart_StringToCString is here - it
does exactly what we need, and the lifetime of the C string is long enough for
it to be printed - the fwrite doesn't need the buffer after it returns.  It
seems to do exactly the needed job, since PrintString is only called from
Logger_PrintString, which contains its EnterScope and ExitScope.

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc#newcode1672
vm/dart_api_impl.cc:1672: intptr_t str_len = Utf8::Length(str_obj);
There is still no way for the user to know the length of the encoding - I think
we have to expose Utf8::Length in the API.  Note that the maximum length is 3 *
length bytes, not 2 * length.

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc#newcode1673
vm/dart_api_impl.cc:1673: if (str_len > *length) {
I think this is not enough.  There is no way for the user to know the actual
desired size of the array - they have to overestimate at 3 * length.  I think
Utf8::Length has to be exposed in the API.  But StringToCString works well, and
its use should be encouraged.  Alternatively, you could convert up to the
available size, and report whether more space is needed.

[cshapiro, 2012-11-01 20:59:30]

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc#newcode1677
vm/dart_api_impl.cc:1677: *length= str_len;
missing horizontal space.

[siva, 2012-11-01 21:23:03]

http://codereview.chromium.org/11275107/diff/1/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/11275107/diff/1/bin/builtin_natives.cc#newcode116
bin/builtin_natives.cc:116: result = Dart_StringToUTF8(str, chars, &length);
Because that would mean we are assuming that the string
object does not have any embedded NUL characters in it.

We do a strlen(chars) on the returned string which might
return a truncated length.

I have changed the API to do a scope allocation of the buffer instead of
requiring the buffer to be allocated by the caller.


On 2012/11/01 20:38:01, Bill Hesse wrote:
> I don't see what the problem is with using Dart_StringToCString is here - it
> does exactly what we need, and the lifetime of the C string is long enough for
> it to be printed - the fwrite doesn't need the buffer after it returns.  It
> seems to do exactly the needed job, since PrintString is only called from
> Logger_PrintString, which contains its EnterScope and ExitScope.

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc#newcode1672
vm/dart_api_impl.cc:1672: intptr_t str_len = Utf8::Length(str_obj);
I have changed the API to do a scope allocation inside here, so the caller does
not have to compute the length.

On 2012/11/01 20:38:01, Bill Hesse wrote:
> There is still no way for the user to know the length of the encoding - I
think
> we have to expose Utf8::Length in the API.  Note that the maximum length is 3
*
> length bytes, not 2 * length.

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc#newcode1673
vm/dart_api_impl.cc:1673: if (str_len > *length) {
Check removed.

On 2012/11/01 20:38:01, Bill Hesse wrote:
> I think this is not enough.  There is no way for the user to know the actual
> desired size of the array - they have to overestimate at 3 * length.  I think
> Utf8::Length has to be exposed in the API.  But StringToCString works well,
and
> its use should be encouraged.  Alternatively, you could convert up to the
> available size, and report whether more space is needed.

http://codereview.chromium.org/11275107/diff/1/vm/dart_api_impl.cc#newcode1677
vm/dart_api_impl.cc:1677: *length= str_len;
On 2012/11/01 20:59:30, cshapiro wrote:
> missing horizontal space.

Done.

[Bill Hesse, 2012-11-02 09:16:00]

LGTM.

http://codereview.chromium.org/11275107/diff/7001/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/11275107/diff/7001/bin/builtin_natives.cc#newc...
bin/builtin_natives.cc:113: // interrupt.
I agree that errors should be propagated.  I believe that just replacing the
print with PropagateError(result) would give better behavior.

[cshapiro, 2012-11-02 17:01:56]

http://codereview.chromium.org/11275107/diff/1/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/11275107/diff/1/bin/builtin_natives.cc#newcode112
bin/builtin_natives.cc:112: length = (length * sizeof(uint16_t));
We should be emitting UTF-8, not CESU-8.

However, the UTF-8 encoder is still assuming that supplementary characters will
appear as a single element in the input stream.  That is no longer true.

I will open a separate bug.

[siva, 2012-11-06 01:35:22]

Anton,

We have had to change the API of
Dart_StringToUTF8 because of some usability problems with
the older version. This CL has the change and I wanted
to co-ordinate submission of this change with you so
that dartium build does not break.

Siva.

http://codereview.chromium.org/11275107/diff/7001/bin/builtin_natives.cc
File bin/builtin_natives.cc (right):

http://codereview.chromium.org/11275107/diff/7001/bin/builtin_natives.cc#newc...
bin/builtin_natives.cc:113: // interrupt.
Agree but probably should be done in a different CL as it requires signature
changes etc.

On 2012/11/02 09:16:00, Bill Hesse wrote:
> I agree that errors should be propagated.  I believe that just replacing the
> print with PropagateError(result) would give better behavior.