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

[Jacek Caban]

On 4/18/22 16:42, Gabriel Ivăncescu wrote:
> 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...


I don't think it's justified and I generally don't like spreading direct 
IDispach[Ex] calls over the code base, especially non-trivial ones like 
this one. In this case, we could simply introduce disp_call_name() 
helper that could use disp_invoke().


>>
>>> +    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.


I see. Still, it could use a separated helper shared with 
Number_toLocaleString instead of calling Number_toLocaleString like that.


Thanks,

Jacek