Fix for issue 7089 (Symbols::New was not quite working correctly

Fix for issue 7089 (Symbols::New was not quite working correctly
when there are non ASCII Latin-1 characters in the string).
Committed: https://code.google.com/p/dart/source/detail?r=15711

[cshapiro, 2012-12-04 00:41:07]

lgtm with important comments

https://codereview.chromium.org/11411341/diff/8/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

https://codereview.chromium.org/11411341/diff/8/vm/dart_api_impl.cc#newcode1565
vm/dart_api_impl.cc:1565: return Api::NewHandle(isolate,
String::NewFromUTF8(utf8_array, length));
This should cause a compile time error as NewFromUTF8 has been renamed to
FromUTF8.

https://codereview.chromium.org/11411341/diff/8/vm/symbols.h
File vm/symbols.h (right):

https://codereview.chromium.org/11411341/diff/8/vm/symbols.h#newcode184
vm/symbols.h:184: // Creates a new Symbol from an array of UTF-8 encoded
characters.
This comment needs to be updated.

  // Creates a new Symbol from an array of Latin-1 encoded characters.

[siva, 2012-12-04 00:55:57]

PTAL

https://codereview.chromium.org/11411341/diff/8/vm/dart_api_impl.cc
File vm/dart_api_impl.cc (right):

https://codereview.chromium.org/11411341/diff/8/vm/dart_api_impl.cc#newcode1565
vm/dart_api_impl.cc:1565: return Api::NewHandle(isolate,
String::NewFromUTF8(utf8_array, length));
On 2012/12/04 00:41:07, cshapiro wrote:
> This should cause a compile time error as NewFromUTF8 has been renamed to
> FromUTF8.

Done.

https://codereview.chromium.org/11411341/diff/8/vm/symbols.h
File vm/symbols.h (right):

https://codereview.chromium.org/11411341/diff/8/vm/symbols.h#newcode184
vm/symbols.h:184: // Creates a new Symbol from an array of UTF-8 encoded
characters.
This has changed some.

On 2012/12/04 00:41:07, cshapiro wrote:
> This comment needs to be updated.
> 
>   // Creates a new Symbol from an array of Latin-1 encoded characters.

[cshapiro, 2012-12-04 01:00:46]

What is the motivation for breaking out the symbol constructors?

I might be missing something but, it seems to me that the templates did that
work previously so this just looks like more code.

https://codereview.chromium.org/11411341/diff/13001/vm/symbols.h
File vm/symbols.h (right):

https://codereview.chromium.org/11411341/diff/13001/vm/symbols.h#newcode181
vm/symbols.h:181: // UTF-8 encoded characters and '\0' is considered a
termination character.
Same TODO here as well?

https://codereview.chromium.org/11411341/diff/13001/vm/symbols.h#newcode185
vm/symbols.h:185: static RawString* NewFromUTF8(const uint8_t* utf8_array,
intptr_t len);
Maybe s/NewFromUTF8/FromUTF8/

?

[siva, 2012-12-04 02:36:16]

https://codereview.chromium.org/11411341/diff/13001/vm/symbols.h
File vm/symbols.h (right):

https://codereview.chromium.org/11411341/diff/13001/vm/symbols.h#newcode181
vm/symbols.h:181: // UTF-8 encoded characters and '\0' is considered a
termination character.
On 2012/12/04 01:00:46, cshapiro wrote:
> Same TODO here as well?

Done.

https://codereview.chromium.org/11411341/diff/13001/vm/symbols.h#newcode185
vm/symbols.h:185: static RawString* NewFromUTF8(const uint8_t* utf8_array,
intptr_t len);
Renamed this to FromUTF8 and all the subsequent
ones to FromLatin1, FromUTF16 and FromUTF32 to make
it more explicit.

The motivation of moving the template function as a private
function was to make the API more explicit.

On 2012/12/04 01:00:46, cshapiro wrote:
> Maybe s/NewFromUTF8/FromUTF8/
> 
> ?

Done.

[cshapiro, 2012-12-04 20:14:11]

thanks, still lgtm