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