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

[Jacek Caban]

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();
> - 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.

Thanks,

Jacek