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

compiled/bindings/generator.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 204 matching lines @@
         return "  " + call_str + ";\n";
       case TypeCategory::INT:
       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";
         result += "  if (result)\n";
         result += "  {\n";
@@ skipping 90 matching lines @@
         {
           buffer = Runtime.stackAlloc(str.length * 2);
           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)
       {
         var result = function(pointer)
@@ skipping 93 matching lines @@
 
 void printBindings()
 {
   bindings_internal::printHelpers();
 
   for (const auto& cls : classes)
     bindings_internal::printClass(cls);
   for (const auto& cls : classes)
     bindings_internal::printClassMapping(cls);
 }