Improve the encoding/decoding to/from system encoding on Windows

Improve the encoding/decoding to/from system encoding on Windows

On Windows the dart:io SYSTEM_ENCODING is now also working with strings
which have null characters in them. Previously the string would be
terminated at the null character.

BUG=https://github.com/dart-lang/sdk/issues/23295
R=kustermann@google.com, lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/0415d9271d50ffd61e5243c391a02678f271bce1

[Ivan Posva, 2015-06-22 08:54:16]

iposva@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2015-06-22 08:54:17]

DBC -ip

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:309: // On some platforms
StringUtils::Utf8ToConsoleString is a no op, and
Comment does not match the call here?

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:313: delete[] buffer;
Wouldn't it be easier and safer to use Dart_ScopeAllocate for these buffers?
Then you can rely on the zone to delete all of these blocks.

[kustermann, 2015-06-22 11:11:22]

The additional 0-termination of the strings (which in return can also contain 0
characters) seems a bit fishy IMHO.

Hoping lrn@ takes also a look at this.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h
File runtime/bin/utils.h (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:68: // converted. If len is -1 convertion until a null
character will ve
ve -> be

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_andro...
File runtime/bin/utils_android.cc (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_andro...
runtime/bin/utils_android.cc:74: return utf8;
There is potential for issues here. Previously the assumption was that
conversion is 0-terminated. Which means we can just return the (hopefully
already 0-terminated) input.

Now if len is not the length of the input string, but rather shorter, we still
return the same string. This seems inconsistent with the windows implementation,
which would make a copy and 0-terminate the shorter string.

tl;dr Do we need to make a shorter 0-terminated copy iff "strlen(utf8) != len" ?

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.cc
File runtime/bin/utils_win.cc (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:71: delete[] wide;
The extra copy is a bit unfortunate :-/

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:79: wchar_t* wide = Utf8ToWide(utf8, len, &wide_len);
Maybe qualify with StringUtils:: as you normally do?

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:82: char* ansi =
reinterpret_cast<char*>(malloc(system_len + 1));
Do we not want to handle allocation failures?

Also: It's inconsistent, sometimes 'new type[len]'/delete is used and sometimes
malloc/free

If you always stick to the C++ variants, you don't need to handle NULL return
values AFAIK

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:112: reinterpret_cast<wchar_t*>(malloc((wide_len + 1) *
sizeof(wchar_t)));
The distinction between the number-of-bytes/number-of-characters is very subtle.
Why not use 'new wchar_t[wide_len + 1]'.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:114: wide[wide_len] = '\0';
Is this common to 0-terminate a 2-byte wide string?

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
File tests/standalone/io/system_encoding_test.dart (right):

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
tests/standalone/io/system_encoding_test.dart:22: test(bytes) {
Please add types to the parameters.

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
tests/standalone/io/system_encoding_test.dart:40: '???'.codeUnits);
Could you add a comment here to explain the difference between windows and the
rest?

[Lasse Reichstein Nielsen, 2015-06-22 12:08:35]

Please update the documentation on SYSTEM_ENCODING in string_transformer.dart. 
It needs to say what a "system encoding" is, and where and when it is used/can
be used.
It seems to me that it's related to console I/O, but that's not documented
anywhere near SystemEncoding.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:334: Dart_Handle external_array =
IOBuffer::Allocate(system_len, &buffer);
Could Utf8ToConsoleString be made to allocate using IOBuffer::Allocate?
It seems wasteful to make it allocate something and then allocate again to move
it to a different part of the memory.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:336: // See comment further down.
Put comment here, say "See comment above for why the system_string is not
freed." below.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:343: // result of the conversion as nothing is allocated.
as nothing new has been allocated.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h
File runtime/bin/utils.h (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:64: // pointer that is different from the input pointer the
returned
comma after second "pointer"

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:67: // If the len argument is passed that number of
characters are
comma after "passed", or add a "then".

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:68: // converted. If len is -1 convertion until a null
character will ve
If len is -1, conversion will stop at the first NUL character.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:69: // performed. If result_len is not NULL this is set to
the number of
this is set -> , it is used to set

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.cc
File runtime/bin/utils_win.cc (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:71: delete[] wide;
But probably unavoidable if we want to use the system MultiByteToWideChar code
to decode the current codepage. I don't think there is a MultiByteToMultiByte
function.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:84: ansi[system_len] = '\0';
Should this only be set when result_len is NULL? 
And not if len is -1 (in that case the terminating NUL is part of the output
already).

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:99: utf8[utf8_len] = '\0';
Again, only include the extra NUL if result_len is NULL and len >= 0.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:112: reinterpret_cast<wchar_t*>(malloc((wide_len + 1) *
sizeof(wchar_t)));
Sadly because the function is documented as returning something allocated with
malloc, so there may be existing code freeing it with free.

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
File tests/standalone/io/system_encoding_test.dart (right):

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
tests/standalone/io/system_encoding_test.dart:40: '???'.codeUnits);
WHat kind of silly encoding is that? :)

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
tests/standalone/io/system_encoding_test.dart:41: } else {
So all non-windows systems use UTF-8? So far. :)

[Søren Gjesse, 2015-06-23 11:18:00]

Thanks for the review.

I have refactored the code to move more Windows specific
functions into utils_win.cc. The conversion functions in
process.cc are only used on Windows, and now they just throw (or
fail with ASSSERT in debug mode) if ever called.

Cleaned up the adding of terminating NUL characters as well. The
Windows functions include the NUL character if not used with an
explicit length.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:309: // On some platforms
StringUtils::Utf8ToConsoleString is a no op, and
On 2015/06/22 08:54:16, Ivan Posva wrote:
> Comment does not match the call here?

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:313: delete[] buffer;
On 2015/06/22 08:54:16, Ivan Posva wrote:
> Wouldn't it be easier and safer to use Dart_ScopeAllocate for these buffers?
> Then you can rely on the zone to delete all of these blocks.

Changed it for the buffer variable used here. We could do the same for the
return value from StringUtils::ConsoleStringToUtf8 and
StringUtils::Utf8ToConsoleString. These two have platform specific
implementations, and right now we never call dart_api functions in platform
specific code. however I don't see any reason for this restriction.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:334: Dart_Handle external_array =
IOBuffer::Allocate(system_len, &buffer);
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> Could Utf8ToConsoleString be made to allocate using IOBuffer::Allocate?
> It seems wasteful to make it allocate something and then allocate again to
move
> it to a different part of the memory.

I don't want Utf8ToConsoleString to allocate in a special way. Another option
could be to just create the external array based on the already returned buffer.
That is for a separate CL.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:336: // See comment further down.
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> Put comment here, say "See comment above for why the system_string is not
> freed." below.

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/process.cc#...
runtime/bin/process.cc:343: // result of the conversion as nothing is allocated.
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> as nothing new has been allocated.

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h
File runtime/bin/utils.h (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:64: // pointer that is different from the input pointer the
returned
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> comma after second "pointer"

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:67: // If the len argument is passed that number of
characters are
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> comma after "passed", or add a "then".

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:68: // converted. If len is -1 convertion until a null
character will ve
On 2015/06/22 11:11:21, kustermann wrote:
> ve -> be

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:68: // converted. If len is -1 convertion until a null
character will ve
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> If len is -1, conversion will stop at the first NUL character. 

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils.h#new...
runtime/bin/utils.h:69: // performed. If result_len is not NULL this is set to
the number of
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> this is set -> , it is used to set

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_andro...
File runtime/bin/utils_android.cc (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_andro...
runtime/bin/utils_android.cc:74: return utf8;
On 2015/06/22 11:11:21, kustermann wrote:
> There is potential for issues here. Previously the assumption was that
> conversion is 0-terminated. Which means we can just return the (hopefully
> already 0-terminated) input.
> 
> Now if len is not the length of the input string, but rather shorter, we still
> return the same string. This seems inconsistent with the windows
implementation,
> which would make a copy and 0-terminate the shorter string.
> 
> tl;dr Do we need to make a shorter 0-terminated copy iff "strlen(utf8) != len"
?

Changed this to be only implemented on Windows. This code path is *never*
reached on other platforms, so I don't want to reason about what to do there.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.cc
File runtime/bin/utils_win.cc (right):

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:79: wchar_t* wide = Utf8ToWide(utf8, len, &wide_len);
On 2015/06/22 11:11:21, kustermann wrote:
> Maybe qualify with StringUtils:: as you normally do?

Done.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:82: char* ansi =
reinterpret_cast<char*>(malloc(system_len + 1));
On 2015/06/22 11:11:21, kustermann wrote:
> Do we not want to handle allocation failures?
> 
> Also: It's inconsistent, sometimes 'new type[len]'/delete is used and
sometimes
> malloc/free
> 
> If you always stick to the C++ variants, you don't need to handle NULL return
> values AFAIK

Added a check here. However we don't handle failed allocations in general.
Regarding C++ new failing I think it also returns NULL. We compile with C++
exceptions turned off, so it will probably not throw bad_alloc.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:84: ansi[system_len] = '\0';
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> Should this only be set when result_len is NULL? 
> And not if len is -1 (in that case the terminating NUL is part of the output
> already).

I removed this, as when len is -1 the NUL char is included in the length from
WideCharToMultiByte.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:99: utf8[utf8_len] = '\0';
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> Again, only include the extra NUL if result_len is NULL and len >= 0.

Removed, see above.

https://codereview.chromium.org/1194883002/diff/40001/runtime/bin/utils_win.c...
runtime/bin/utils_win.cc:114: wide[wide_len] = '\0';
On 2015/06/22 11:11:21, kustermann wrote:
> Is this common to 0-terminate a 2-byte wide string?

It is, all the w versions of string functions on Windows that deal with wchar_t
(e.g. wcslen) is using NUL termination.

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
File tests/standalone/io/system_encoding_test.dart (right):

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
tests/standalone/io/system_encoding_test.dart:22: test(bytes) {
On 2015/06/22 11:11:21, kustermann wrote:
> Please add types to the parameters.

Done.

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
tests/standalone/io/system_encoding_test.dart:40: '???'.codeUnits);
On 2015/06/22 11:11:21, kustermann wrote:
> Could you add a comment here to explain the difference between windows and the
> rest?

Done.

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
tests/standalone/io/system_encoding_test.dart:40: '???'.codeUnits);
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> WHat kind of silly encoding is that? :)

That is what you get!

https://codereview.chromium.org/1194883002/diff/40001/tests/standalone/io/sys...
tests/standalone/io/system_encoding_test.dart:41: } else {
On 2015/06/22 12:08:34, Lasse Reichstein Nielsen wrote:
> So all non-windows systems use UTF-8? So far. :)

Yes, added comment.

[kustermann, 2015-06-23 12:03:58]

lgtm - but let the others also take a look

https://codereview.chromium.org/1194883002/diff/120001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/1194883002/diff/120001/runtime/bin/process.cc...
runtime/bin/process.cc:312: if
(Dart_IsError(result))Dart_PropagateError(result);
space after )

https://codereview.chromium.org/1194883002/diff/120001/runtime/bin/utils_win.cc
File runtime/bin/utils_win.cc (right):

https://codereview.chromium.org/1194883002/diff/120001/runtime/bin/utils_win....
runtime/bin/utils_win.cc:84: if (ansi == NULL) return NULL;
This early return is not doing "free(wide)"

[Søren Gjesse, 2015-06-23 12:13:36]

https://codereview.chromium.org/1194883002/diff/120001/runtime/bin/process.cc
File runtime/bin/process.cc (right):

https://codereview.chromium.org/1194883002/diff/120001/runtime/bin/process.cc...
runtime/bin/process.cc:312: if
(Dart_IsError(result))Dart_PropagateError(result);
On 2015/06/23 12:03:58, kustermann wrote:
> space after )

Done.

https://codereview.chromium.org/1194883002/diff/120001/runtime/bin/utils_win.cc
File runtime/bin/utils_win.cc (right):

https://codereview.chromium.org/1194883002/diff/120001/runtime/bin/utils_win....
runtime/bin/utils_win.cc:84: if (ansi == NULL) return NULL;
On 2015/06/23 12:03:58, kustermann wrote:
> This early return is not doing "free(wide)"

Done.

[Lasse Reichstein Nielsen, 2015-06-23 13:08:51]

lgtm

https://codereview.chromium.org/1194883002/diff/140001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/1194883002/diff/140001/runtime/bin/file_win.c...
runtime/bin/file_win.cc:577: free(const_cast<wchar_t*>(system_name));
Could it be useful to add a 
  template<typename t>
  inline void free(const t* malloced) { free(const_cast<t*>(malloced); }
utility function somewhere? (where the C++ code may not be valid, but I hope you
get the idea.

Alternatively, is there a way to make the argument not be a const pointer (since
you are modifiying its content anyway when you free it)?

https://codereview.chromium.org/1194883002/diff/140001/sdk/lib/io/string_tran...
File sdk/lib/io/string_transformer.dart (right):

https://codereview.chromium.org/1194883002/diff/140001/sdk/lib/io/string_tran...
sdk/lib/io/string_transformer.dart:10: /// communicating on stdin/stdout and
stderr.
-> stdin, stdout and stderr.

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
File tests/standalone/io/system_encoding_test.dart (right):

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
tests/standalone/io/system_encoding_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
2015?

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
tests/standalone/io/system_encoding_test.dart:10: void testEncodeDecode(String
str) {
Document that this only works for ASCII.

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
tests/standalone/io/system_encoding_test.dart:16: void
testDecodeEncode(List<int> bytes) {
... and this should work for all bytes.

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
tests/standalone/io/system_encoding_test.dart:47:
SYSTEM_ENCODING.encoder.convert('\u1234\u5678\u9abc'),
.encoder.convert(...) -> .encode(...)

Simlar above and below.

[Søren Gjesse, 2015-06-24 07:58:26]

https://codereview.chromium.org/1194883002/diff/140001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/1194883002/diff/140001/runtime/bin/file_win.c...
runtime/bin/file_win.cc:577: free(const_cast<wchar_t*>(system_name));
On 2015/06/23 13:08:50, Lasse Reichstein Nielsen wrote:
> Could it be useful to add a 
>   template<typename t>
>   inline void free(const t* malloced) { free(const_cast<t*>(malloced); }
> utility function somewhere? (where the C++ code may not be valid, but I hope
you
> get the idea.
> 
> Alternatively, is there a way to make the argument not be a const pointer
(since
> you are modifiying its content anyway when you free it)?

One option to get rid of the const is to get rid of it in the allocation, that
is change

const wchar_t* system_name = StringUtilsWin::Utf8ToWide(pathname)

to

wchar_t* system_name =
StringUtilsWin::Utf8ToWide(const_cast<wchat_t*>(pathname))

I am going to leave it for now, as it is all over the place.

https://codereview.chromium.org/1194883002/diff/140001/sdk/lib/io/string_tran...
File sdk/lib/io/string_transformer.dart (right):

https://codereview.chromium.org/1194883002/diff/140001/sdk/lib/io/string_tran...
sdk/lib/io/string_transformer.dart:10: /// communicating on stdin/stdout and
stderr.
On 2015/06/23 13:08:50, Lasse Reichstein Nielsen wrote:
> -> stdin, stdout and stderr.

Done.

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
File tests/standalone/io/system_encoding_test.dart (right):

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
tests/standalone/io/system_encoding_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2015/06/23 13:08:51, Lasse Reichstein Nielsen wrote:
> 2015?

Done.

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
tests/standalone/io/system_encoding_test.dart:10: void testEncodeDecode(String
str) {
On 2015/06/23 13:08:50, Lasse Reichstein Nielsen wrote:
> Document that this only works for ASCII.

Done.

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
tests/standalone/io/system_encoding_test.dart:16: void
testDecodeEncode(List<int> bytes) {
On 2015/06/23 13:08:50, Lasse Reichstein Nielsen wrote:
> ... and this should work for all bytes.

No, only 0..127, as on all platforms but Windows this is UTF-8.
Documented that

https://codereview.chromium.org/1194883002/diff/140001/tests/standalone/io/sy...
tests/standalone/io/system_encoding_test.dart:47:
SYSTEM_ENCODING.encoder.convert('\u1234\u5678\u9abc'),
On 2015/06/23 13:08:51, Lasse Reichstein Nielsen wrote:
> .encoder.convert(...) -> .encode(...)
> 
> Simlar above and below.

Done.

[Søren Gjesse, 2015-06-24 08:36:17]

Committed patchset #9 (id:160001) manually as
0415d9271d50ffd61e5243c391a02678f271bce1.