[PATCH v5 02/11] jscript: Use to_primitive when getting the default value for js objects.

[Gabriel Ivăncescu]

On 22/11/2021 16:41, Jacek Caban wrote:
> On 11/22/21 1:52 PM, Gabriel Ivăncescu wrote:
>> On 22/11/2021 14:22, Jacek Caban wrote:
>>> Hi Gabriel,
>>>
>>> On 11/19/21 7:03 PM, Gabriel Ivăncescu wrote:
>>>> diff --git a/dlls/jscript/jscript.h b/dlls/jscript/jscript.h
>>>> index 69897cd..d37fbd1 100644
>>>> --- a/dlls/jscript/jscript.h
>>>> +++ b/dlls/jscript/jscript.h
>>>> @@ -230,6 +230,9 @@ typedef struct {
>>>>       builtin_setter_t setter;
>>>>   } builtin_prop_t;
>>>> +HRESULT 
>>>> jsdisp_builtin_get_default_value(script_ctx_t*,jsdisp_t*,jsval_t*) 
>>>> DECLSPEC_HIDDEN;
>>>> +#define JSDISP_DEFINE_BUILTIN_VALUE(value) {NULL, value,0, 
>>>> jsdisp_builtin_get_default_value}
>>>
>>>
>>> Could we just handle DISPID_VALUE in dispex.c callers without using 
>>> builtin_prop_t at all?
>>>
>>>
>>> Thanks,
>>>
>>> Jacek
>>>
>>
>> How are we going to handle the methods then, for the jsdisps that have 
>> them? For example, Function_value. Wouldn't it require special-casing 
>> them? I thought it's less "elegant" since it doesn't re-use the same 
>> code path as now, just different builtin data.
> 
> 
> I meant it only for getter, Function_value is for calling DISPID_VALUE. 
> If getter is the same in every builtin_info_t, then it means that it's 
> redundant, which is not really elegant. DISPID_VALUE is special in many 
> ways, I don't see what's wrong with handling it separately.
> 
> 
> Jacek
> 

Well, it's not redundant since it's only for DISPID_VALUE props, but the 
code handling the getters is universal to *all* props not just 
DISPID_VALUE, which of course is not always the same. Right now we don't 
add any code and just re-use the existing prop code.

That said, if you still aren't convinced, I have a question. Should I 
remove the value_prop completely from the builtin_info_t and from the 
prop list? Right now it's always the first prop, i.e. zero, to match 
DISPID_VALUE.

Then, since I have to special-case it anyway, I'll just add a 
builtin_invoke_t value_invoke instead and call it if needed in InvokeEx, 
without having an actual prop for it.

Or do you want to keep the value_prop like now, and always have it NULL 
getter, and on top of that add special handling in InvokeEx? imo this 
adds more redundancy and extra code, but it's up to you.

Which choice should I go with?

Thanks,
Gabriel