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

runtime/bin/process.cc

 // Copyright (c) 2013, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "bin/dartutils.h"
 #include "bin/io_buffer.h"
 #include "bin/platform.h"
 #include "bin/process.h"
 #include "bin/socket.h"
 #include "bin/utils.h"
@@ skipping 280 matching lines @@
   intptr_t bytes_length = 0;
   Dart_Handle result = Dart_ListLength(bytes, &bytes_length);
   if (Dart_IsError(result)) Dart_PropagateError(result);
   uint8_t* buffer = new uint8_t[bytes_length + 1];
   result = Dart_ListGetAsBytes(bytes, 0, buffer, bytes_length);
   buffer[bytes_length] = '\0';
   if (Dart_IsError(result)) {
     delete[] buffer;
     Dart_PropagateError(result);
   }
+  intptr_t len;
   char* str =
-      StringUtils::ConsoleStringToUtf8(reinterpret_cast<char*>(buffer));
-  Dart_SetReturnValue(args, DartUtils::NewString(str));
+      StringUtils::ConsoleStringToUtf8(
+          reinterpret_cast<char*>(buffer),
+          bytes_length,
+          &len);
+  result =
+      Dart_NewStringFromUTF8(reinterpret_cast<const uint8_t*>(str), len);
+  // On some platforms StringUtils::Utf8ToConsoleString is a no op, and

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

Comment does not match the call here?

[Søren Gjesse, 2015/06/23 11:17:59]

Done.

+  // the data to convert is passed through. In that case don't free the
+  // result of the conversion as nothing is allocated.
   if (str != reinterpret_cast<char*>(buffer)) free(str);
+  delete[] buffer;

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

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.

[Søren Gjesse, 2015/06/23 11:17:59]

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.

+  if (Dart_IsError(result)) {
+    Dart_PropagateError(result);
+  }
+  Dart_SetReturnValue(args, result);
 }
 
 
 void FUNCTION_NAME(StringToSystemEncoding)(Dart_NativeArguments args) {
   Dart_Handle str = Dart_GetNativeArgument(args, 0);
-  const char* utf8 = DartUtils::GetStringValue(str);
-  const char* system_string = StringUtils::Utf8ToConsoleString(utf8);
-  int external_length = strlen(system_string);
+  char* utf8;
+  intptr_t utf8_len;
+  Dart_Handle result = Dart_StringToUTF8(
+      str, reinterpret_cast<uint8_t **>(&utf8), &utf8_len);
+  if (Dart_IsError(result)) {
+    Dart_PropagateError(result);
+  }
+  intptr_t system_len;
+  const char* system_string =
+      StringUtils::Utf8ToConsoleString(utf8, utf8_len, &system_len);
   uint8_t* buffer = NULL;
-  Dart_Handle external_array = IOBuffer::Allocate(external_length, &buffer);
-  memmove(buffer, system_string, external_length);
+  Dart_Handle external_array = IOBuffer::Allocate(system_len, &buffer);

[Lasse Reichstein Nielsen, 2015/06/22 12:08:34]

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.

[Søren Gjesse, 2015/06/23 11:17:58]

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.

+  if (Dart_IsError(external_array)) {
+    // See comment further down.

[Lasse Reichstein Nielsen, 2015/06/22 12:08:34]

Put comment here, say "See comment above for why the system_string is not
freed." below.

[Søren Gjesse, 2015/06/23 11:17:58]

Done.

+    if (utf8 != system_string) free(const_cast<char*>(system_string));
+    Dart_PropagateError(result);
+  }
+  memmove(buffer, system_string, system_len);
+  // On some platforms StringUtils::Utf8ToConsoleString is a no op, and
+  // the data to convert is passed through. In that case don't free the
+  // result of the conversion as nothing is allocated.

[Lasse Reichstein Nielsen, 2015/06/22 12:08:34]

as nothing new has been allocated.

[Søren Gjesse, 2015/06/23 11:17:59]

Done.

   if (utf8 != system_string) free(const_cast<char*>(system_string));
   Dart_SetReturnValue(args, external_array);
 }
 
 }  // namespace bin
 }  // namespace dart