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

Current code works mostly by coincidence - it always calls DependentString constructor regardless of whether DependentString or OwnedString instance is being created. With this change we should be calling the right constructor so that we don't run into issues later.

[Wladimir Palant, 2017-05-09 10:39:30]

Current code works mostly by coincidence - it always calls DependentString
constructor regardless of whether DependentString or OwnedString instance is
being created. With this change we should be calling the right constructor so
that we don't run into issues later.

[sergei, 2017-08-23 10:06:16]

LGTM

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
compiled/bindings/generator.cpp:230: return result;
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.

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
compiled/bindings/generator.cpp:237: result += "  var result =
readString(string);\n";
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]

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
compiled/bindings/generator.cpp:230: return result;
On 2017/08/23 10:06:15, sergei wrote:
> 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.

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

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
compiled/bindings/generator.cpp:237: result += "  var result =
readString(string);\n";
On 2017/08/23 10:06:15, sergei wrote:
> 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.

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:20]

LGTM

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
compiled/bindings/generator.cpp:230: return result;
On 2017/08/23 10:26:41, Wladimir Palant wrote:
> On 2017/08/23 10:06:15, sergei wrote:
> > 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.
> 
> Ok, I added a comment. In fact, we might need to call the destructor in debug
> mode if we add reference counting.

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.

https://codereview.adblockplus.org/29434561/diff/29434562/compiled/bindings/g...
compiled/bindings/generator.cpp:237: result += "  var result =
readString(string);\n";
On 2017/08/23 10:26:41, Wladimir Palant wrote:
> On 2017/08/23 10:06:15, sergei wrote:
> > 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.
> 
> I tried that but it makes the code here less obvious IMHO. The few bytes saved
> don't really matter either.

Acknowledged.