[PATCH 02/10] jscript: Store the necessary function and variable info in the TypeInfo.

[Gabriel Ivăncescu]

Hi Jacek,

On 12/10/19 7:49 PM, Jacek Caban wrote:
> Hi Gabriel,
> 
> On 12/9/19 6:12 PM, Gabriel Ivăncescu wrote:
>>   static DWORD get_flags(jsdisp_t *This, dispex_prop_t *prop)
>>   {
>>       if(prop->type == PROP_PROTREF) {
>> @@ -590,9 +604,21 @@ static HRESULT fill_protrefs(jsdisp_t *This)
>>       return S_OK;
>>   }
>> +struct typeinfo_func {
>> +    dispex_prop_t *prop;
>> +    jsdisp_t *disp;
>> +};
> 
> 
> Storing the whole function object reference does not seem right here. 
> Maybe we should store function_code_t instead? We could have a single 
> getter that could replace both is_source_function() and 
> get_source_function_params() from your patches. Such getter should 
> probably use function_vtbl_t for that. Storing bytecode_t pointer inside 
> function_code_t could also be handy for reference tracking.
> 
> 

Ah sure, storing the function object seemed the easiest for me. I think 
I got the hang how this works now after looking at it for a bit longer, 
so I'll do as you suggested :-)

>> +    for (prop = This->props, end = prop + This->prop_cnt; prop != 
>> end; prop++)
>> +    {
>> +        if (!prop->name || prop->type != PROP_JSVAL || !(prop->flags 
>> & PROPF_ENUMERABLE))
>> +            continue;
>> +
>> +        /* If two identifiers differ only by case, the TypeInfo fails */
>> +        pos = This->props[get_props_idx(This, prop->hash)].bucket_head;
>> +        while (pos)
>> +        {
>> +            cur = This->props + pos;
>> +
>> +            if (prop->hash == cur->hash && prop != cur &&
>> +                cur->type == PROP_JSVAL && (cur->flags & 
>> PROPF_ENUMERABLE) &&
>> +                !wcsicmp(prop->name, cur->name))
>> +            {
>> +                return TYPE_E_AMBIGUOUSNAME;
>> +            }
>> +            pos = cur->bucket_next;
>> +        }
> 
> 
> It would be good to limit dispex_prop_t usage inside ITypeInfo in 
> general. IDispatchEx support in jscript deserves pretty deep changes and 
> extending usage of its internal structure may not be helpful. I'm fine 
> with trying to use them here for now, but I'd rather avoid depending on 
> its internals and that's the kind of internal thing I'd rather not have 
> here. We could call GetDispID(fdexNameCaseInsensitive) and compare 
> returned id to current one, if we had support for that. However, is it 
> really important in practice? I wouldn't mind leaving it as FIXME for now.
> 
> 

Are you referring to the hash lookup? If so, I think this place could be 
an exception, since it's also part of the DispatchEx_* implementation, 
anyway, so it shouldn't be an issue using internals.

The main reason is that it simplifies the code. Leaving it as a FIXME 
would complicate the rest of the code that assumes there are no 
duplicated names, case insensitively.

I don't think GetDispID(fdexNameCaseInsensitive) would work in all 
cases: there's a chance that it returns the same DISPID, if it's the 
first one found. I'm also not sure if it even works on jscript (it might 
enforce case sensitivity due to language specs, depends on tests).

However, if you were referring to the other usage of prop (such as 
flags), that's actually quite needed else it will fail to pass the tests 
since it's not exclusive enough.


IMO, we should keep it this way, since it should be the only "new" place 
it's used, and it's implemented directly in DispatchEx_GetTypeInfo.

Thanks,
Gabriel