Noissue - [emscripten] Call the correct constructor for strings created from JavaScript

compiled/bindings/generator.cpp

Index: compiled/bindings/generator.cpp
===================================================================
--- a/compiled/bindings/generator.cpp
+++ b/compiled/bindings/generator.cpp
@@ -217,24 +217,30 @@ namespace bindings_internal
       case TypeCategory::FLOAT:
       case TypeCategory::DOUBLE:
         return "  var result = " + call_str + ";\n";
       case TypeCategory::INT64:
         return "  var result = Runtime.makeBigInt(" + call_str + ", " +
                                                   "Runtime.getTempRet0(), " +
                                                   "true);\n";
       case TypeCategory::DEPENDENT_STRING:
-      case TypeCategory::OWNED_STRING:
       {
         std::string result;
         result += "  var string = createString();\n";
         result += "  " + call_str + ";\n";
         result += "  var result = readString(string);\n";
-        if (call.returnType == TypeCategory::OWNED_STRING)
-          result += "  Module._DestroyString(string);\n";
+        return result;

[sergei, 2017/08/23 10:06:15]

Strictly speaking, despite destructor of DependentString does nothing it is
still should be called or we should put a comment here that it's an
optimization. It seems another issue, though, but can be done here.

[Wladimir Palant, 2017/08/23 10:26:41]

Ok, I added a comment. In fact, we might need to call the destructor in debug
mode if we add reference counting.

[sergei, 2017/08/23 10:39:19]

Yeah, I thought about similar debug needs too. If it's possible we could maybe
have that call optional. The current variant is fine for me, it can be added
when there is a real demand for that.

+      }
+      case TypeCategory::OWNED_STRING:
+      {
+        std::string result;
+        result += "  var string = createOwnedString();\n";
+        result += "  " + call_str + ";\n";
+        result += "  var result = readString(string);\n";

[sergei, 2017/08/23 10:06:15]

Since anyway we have separate calls of Module._InitOwnedString(result); and
Module._DestroyString(string); we could move the latter into a readString which
calls Module._DestroyString(string);. It could save few bytes of JS code.

[Wladimir Palant, 2017/08/23 10:26:41]

I tried that but it makes the code here less obvious IMHO. The few bytes saved
don't really matter either.

[sergei, 2017/08/23 10:39:19]

Acknowledged.

+        result += "  Module._DestroyString(string);\n";
         return result;
       }
       case TypeCategory::STRING_REF:
         return "  var result = readString(" + call_str + ");\n";
       case TypeCategory::CLASS_PTR:
       {
         std::string result;
         result += "  var result = " + call_str + ";\n";
@@ -335,16 +341,23 @@ namespace bindings_internal
           length = copyString(str, buffer);
         }
 
         var result = Runtime.stackAlloc(sizeofString);
         Module._InitString(result, buffer, length);
         return result;
       }
 
+      function createOwnedString()
+      {
+        var result = Runtime.stackAlloc(sizeofString);
+        Module._InitOwnedString(result);
+        return result;
+      }
+
       function readString(str)
       {
         var length = Module._GetStringLength(str);
         var pointer = Module._GetStringData(str) >> 1;
         return String.fromCharCode.apply(String, HEAP16.slice(pointer, pointer + length));
       }
 
       function createClass(superclass, ref_counted_offset, props)