Re: [PATCH 4/8] jscript: Implement Array.prototype.toLocaleString.

[Gabriel Ivăncescu]

Hi Jacek,

Sorry for delay on this, been a bit swamped.

On 15/04/2022 21:02, Jacek Caban wrote:
> Hi Gabriel,
>
> On 4/15/22 15:00, Gabriel Ivăncescu wrote:
> > +static HRESULT to_locale_string(script_ctx_t *ctx, jsval_t val, 
> > jsstr_t **str)
> > +{
> > +    DISPPARAMS dp = { 0 };
> > +    IDispatchEx *dispex;
> > +    jsdisp_t *jsdisp;
> > +    IDispatch *obj;
> > +    EXCEPINFO ei;
> > +    HRESULT hres;
> > +    UINT err = 0;
> > +    VARIANT var;
> > +    DISPID id;
> > +    BSTR bstr;
> > -
> >
> > +    switch(jsval_type(val)) {
> > +    case JSV_OBJECT:
> > +        obj = get_object(val);
> > +        if((jsdisp = to_jsdisp(obj))) {
> > +            hres = jsdisp_call_name(jsdisp, L"toLocaleString", 
> > DISPATCH_METHOD, 0, NULL, &val);
> > +            if(FAILED(hres)) {
> > +                if(hres == JS_E_INVALID_PROPERTY && ctx->version >= 
> > SCRIPTLANGUAGEVERSION_ES5)
> > +                    hres = JS_E_FUNCTION_EXPECTED;
> > +                return hres;
> > +            }
> > +            break;
> > +        }
> > -
> >
> > +        if(!(bstr = SysAllocString(L"toLocaleString")))
> > +            return E_OUTOFMEMORY;
> > -
> >
> > +        V_VT(&var) = VT_EMPTY;
> > +        hres = IDispatch_QueryInterface(obj, &IID_IDispatchEx, 
> > (void**)&dispex);
> > +        if(SUCCEEDED(hres) && dispex) {
> > +            hres = IDispatchEx_GetDispID(dispex, bstr, 
> > make_grfdex(ctx, fdexNameCaseSensitive), &id);
> > +            if(SUCCEEDED(hres)) {
> > +                hres = IDispatchEx_InvokeEx(dispex, id, ctx->lcid, 
> > DISPATCH_METHOD, &dp, &var, &ei, &ctx->jscaller->IServiceProvider_iface);
> > +                if(hres == DISP_E_EXCEPTION)
> > +                    disp_fill_exception(ctx, &ei);
> > +            }
> > +            IDispatchEx_Release(dispex);
> > +        }else {
> > +            hres = IDispatch_GetIDsOfNames(obj, &IID_NULL, &bstr, 1, 
> > 0, &id);
> > +            if(SUCCEEDED(hres)) {
> > +                hres = IDispatch_Invoke(obj, id, &IID_NULL, 
> > ctx->lcid, DISPATCH_METHOD, &dp, &var, &ei, &err);
> > +                if(hres == DISP_E_EXCEPTION)
> > +                    disp_fill_exception(ctx, &ei);
> > +            }
> > +        }
> > +        SysFreeString(bstr);
> > +        if(FAILED(hres))
> > +            return hres;
> > -
> >
> > +        hres = variant_to_jsval(ctx, &var, &val);
> > +        VariantClear(&var);
> > +        break;
>
> Shouldn't it just use disp_call();

disp_call needs a DISPID though. I still need to obtain the DISPID, so 
just using Invoke after doesn't seem like much, considering I already 
QueryInterface for IDispatchEx for the DISPID...
> > +    case JSV_NUMBER:
> > +        if(ctx->version >= SCRIPTLANGUAGEVERSION_ES5) {
> > +            hres = Number_toLocaleString(ctx, val, 0, 0, NULL, &val);
> > +            if(SUCCEEDED(hres))
> > +                *str = get_string(val);
> > +            return hres;
> > +        }
> > +        /* fall through */
>
> Should it just use the default case? If not, then please don't do direct 
> calls to functions like Number_toLocaleString like this, a separated 
> helper would be better.

According to the tests, it uses Number.prototype.toLocaleString on ES5 
for some reason. See the /* primitive number value not affected */ part.