Re: jscript: Add VT_I2 support

[Jacek Caban]

Hi Alistair,

On 03/13/12 10:36, Alistair Leslie-Hughes wrote:
> Hi,
>
> Changelog:
>     jscript: Add VT_I2 support

static inline BOOL is_num_vt(enum VARENUM vt)
 {
-    return vt == VT_I4 || vt == VT_R8;
+    return vt == VT_I4 || vt == VT_I2 ||vt == VT_R8;
 }


I'm not sure we want this handled this way. I'd much rather make sure VT_I2 (which is not internal jscript type, so it may come only from external/host objects) is never supposed to return TRUE here. If it's unavoidable, it requires convincing tests.


 static inline DOUBLE num_val(const VARIANT *v)
 {
-    return V_VT(v) == VT_I4 ? V_I4(v) : V_R8(v);
+    switch(V_VT(v))
+    {
+       case VT_I2:
+          return V_I2(v);
+       case VT_I4:
+          return V_I4(v);
+       case VT_R8:
+          return V_R8(v);
+    }
+
+    return 0;
 }

It's similar here, I don't think we want to allow num_val calls on VT_I2.

@@ -187,6 +187,7 @@ HRESULT to_primitive(script_ctx_t *ctx, VARIANT *v, jsexcept_t *ei, VARIANT *ret
     case VT_EMPTY:
     case VT_NULL:
     case VT_BOOL:
+    case VT_I2:

That seems like a likely place for VT_I2->VT_I4 conversion.

@@ -569,6 +574,9 @@ HRESULT to_string(script_ctx_t *ctx, VARIANT *v, jsexcept_t *ei, BSTR *str)
     case VT_NULL:
         *str = SysAllocString(nullW);
         break;
+    case VT_I2:
+        *str = int_to_bstr(V_I2(v));
+        break;


I'd like to see it covered by tests.

@@ -76,7 +76,9 @@ static HRESULT Number_toString(script_ctx_t *ctx, vdisp_t *jsthis, WORD flags, D
             return throw_type_error(ctx, ei, JS_E_INVALIDARG, NULL);
     }
 
-    if(V_VT(&number->num) == VT_I4)
+    if(V_VT(&number->num) == VT_I2)
+        val = V_I2(&number->num);
+    else if(V_VT(&number->num) == VT_I4)

I'd much rather keep the assumption that Number stores only VT_I4 or VT_R8.

+objI2 = createVT_I2();
+ok(getVT(objI2) === "VT_I4", "getVT(obj) = " + getVT(objI2));

Hmm, this together with this:

@@ -603,6 +608,7 @@ static HRESULT WINAPI Global_InvokeEx(IDispatchEx *iface, DISPID id, LCID lcid,
         case VT_NULL:
             V_BSTR(pvarRes) = a2bstr("VT_NULL");
             break;
+        case VT_I2:
         case VT_I4:
             V_BSTR(pvarRes) = a2bstr("VT_I4");
             break;

loses the point of this test.

+ok(objI2.toString() === "2", "obj = " + objI2.toString());
+function funcLoopI2()
+{
+    var i = 0;
+    /* Tests: to_primitive, to_number */
+    for(; i < objI2; i++)
+       ;
+    ok(i == objI2, "i(" + i + ") != 1");
+}
+
+funcLoopI2();

These tests don't really show much about how VT_I2 is handled. Please try to make them striker and closer to implementation details. Also objI2 is not really a nice name for something that's not an object.

+    if(!strcmp_wa(bstrName, "createVT_I2")) {
+        *pid = DISPID_GLOBAL_CREATE_I2;
+        return S_OK;
+    }

A better name would be nice.


Cheers,
Jacek