Re: [PATCH 2/3] jscript: Changed to_primitive implementation

[Jacek Caban]

Hi Piotr,

Piotr Caban wrote:
> dlls/jscript/date.c        |    2 +-
>  dlls/jscript/dispex.c      |    2 +-
>  dlls/jscript/engine.c      |   12 ++++----
>  dlls/jscript/jscript.h     |    9 ++++++-
>  dlls/jscript/jsutils.c     |   57
> ++++++++++++++++++++++++++++++++++++++++----
>  dlls/jscript/tests/api.js  |    2 +
>  dlls/jscript/tests/lang.js |    7 +++++
>  7 files changed, 77 insertions(+), 14 deletions(-)

+        hres = jsdisp_get_id(jsdisp, hint == HINT_STRING ? toStringW : valueOfW, 0, &id);
+        if(SUCCEEDED(hres)) {
+            hres = jsdisp_call(jsdisp, id, ctx->lcid, DISPATCH_METHOD, &dp, ret, ei, NULL /*FIXME*/);
+            if(FAILED(hres)) {
+                FIXME("throw TypeError\n");
+                return hres;

You leak jsdisp here.

+            }
+            else if(V_VT(ret) != VT_DISPATCH) {
+                jsdisp_release(jsdisp);
+                return S_OK;
+            }
+        }

You leak V_DISPATCH(ret) here.

+        hres = jsdisp_get_id(jsdisp, hint == HINT_STRING ? valueOfW : toStringW, 0, &id);
+        if(SUCCEEDED(hres)) {
+            hres = jsdisp_call(jsdisp, id, ctx->lcid, DISPATCH_METHOD, &dp, ret, ei, NULL /*FIXME*/);
+            if(FAILED(hres)) {
+                FIXME("throw TypeError\n");
+                return hres;
+            }

You leak jsdisp here.

+            else if(V_VT(ret) != VT_DISPATCH) {
+                jsdisp_release(jsdisp);
+                return S_OK;
+            }
+        }

+date = new Date();
+function toString() {
+    return "toString";
+}
+date.toString = toString;
+ok(""+date === "toString", """+date = " + date);

It would be nice to add a few more tests for different cases like returning objects from toString implementation. Also adding toString function to global scope is asking for a trouble in future tests, so I'd suggest construct like

date.toString = function() {...}

Thanks,
    Jacek