[PATCH v7 08/11] jscript: Get rid of the value_prop from jsdisp props.

[Jacek Caban]

On 11/24/21 6:53 PM, Gabriel Ivăncescu wrote:
> On 24/11/2021 19:36, Jacek Caban wrote:
>> Hi Gabriel,
>>
>> On 11/24/21 3:10 PM, Gabriel Ivăncescu wrote:
>>> @@ -2088,7 +2081,10 @@ HRESULT disp_call(script_ctx_t *ctx, 
>>> IDispatch *disp, DISPID id, WORD flags, uns
>>>           if(ctx != jsdisp->ctx)
>>>               flags &= ~DISPATCH_JSCRIPT_INTERNAL_MASK;
>>> -        hres = jsdisp_call(jsdisp, id, flags, argc, argv, ret);
>>> +        if(id == DISPID_VALUE)
>>> +            hres = jsdisp_call_value(jsdisp, to_disp(jsdisp), 
>>> flags, argc, argv, ret);
>>> +        else
>>> +            hres = jsdisp_call(jsdisp, id, flags, argc, argv, ret);
>>
>>
>> Why do we need it here? Do we ever call disp_call(DISPID_VALUE)?
>>
>
> Yes, apparently it is needed and can happen, otherwise some of the 
> mshtml tests will fail. I added an assertion when testing it and it 
> triggered. This happens from engine.c's exprval_call of course.


That seems weird, is it with jdsidp_t? Since we should never return 
DISPID_VALUE as a property ID, I would not expect us to call it like 
that. It's probably worth a closer look.


>>
>> Also, with your implementation, get_prop(DISPID_VALUE) works because 
>> of integer overflow, but it would be nice to restructure it to not 
>> depend on that.
>>
>
> Oh, I thought it was an idiomatic pattern to check unsigned sizes 
> without having to worry about negative values—since it's specified in 
> the standard. (which is why sizeof() returns an unsigned type)
>
> i.e. if(index < count) is guaranteed to fit within bounds if it's 
> unsigned comparison.
>
> I thought it would simplify the code, do you really want to avoid it?


I meant the conversion from abstraction (which is also guaranteed but 
well...). Anyway, it's not a big deal.


Jacek