[PATCH v2 2/3] windows.globalization: Implement IGlobalizationPreferencesStatics::Languages.

[Rémi Bernon]

On 3/26/21 10:27 AM, Jacek Caban wrote:
> On 3/26/21 10:05 AM, Rémi Bernon wrote:
>> On 3/25/21 10:46 PM, Jacek Caban wrote:
>>> Hi Rémi,
>>>
>>> It looks mostly good, but error handling nuances could be improved.
>>>
>>> On 3/25/21 9:17 AM, Rémi Bernon wrote:
>>>> +static HRESULT STDMETHODCALLTYPE 
>>>> hstring_vector_GetMany(IVectorView_HSTRING *iface,
>>>> +        ULONG start_index, ULONG items_size, HSTRING *items, UINT 
>>>> *count)
>>>> +{
>>>> +    struct hstring_vector *impl = 
>>>> impl_from_IVectorView_HSTRING(iface);
>>>> +    HRESULT hr;
>>>> +    ULONG i;
>>>> +
>>>> +    TRACE("iface %p, start_index %#x, items %p, count %p.\n", 
>>>> iface, start_index, items, count);
>>>> +
>>>> +    memset(items, 0, items_size * sizeof(HSTRING *));
>>>> +
>>>> +    for (i = start_index; i < impl->count && i < start_index + 
>>>> items_size; ++i)
>>>> +        if (FAILED(hr = WindowsDuplicateString(impl->values[i], 
>>>> items + i - start_index)))
>>>> +            return hr;
>>>
>>> This leaks previously allocated strings in error case.
>>>
>>
>> Sure, then in this particular case the largest vector has one element, 
>> so it's not actually leaking anything.
> 
> Sure, but if you want to assume that, then you don't need the loop at 
> all. If you're making things generic, let's make it right (and from the 
> looks of it, it's likely to be useful when implementation of the DLL is 
> more complete).
> 
>> Also, the elements have not been created with 
>> WindowsCreateStringReference, so WindowsDuplicateString will only 
>> increments the string refcount count and is never going to fail.
> 
> Okay, but if you can assume that, then the error check is not needed at 
> all.
> 
>>>> +    WindowsCreateString(locale, wcslen(locale), &hstring);
>>>
>>> This may fail.
>>>
>>
>> Here the only possible error is an allocation error. I honestly expect 
>> that things are going to be pretty bad very soon if we fail to 
>> allocate a few bytes. But yes, the status should be returned.
> 
> Yes, it's largely about consistency: you check for similar errors in 
> other places.
> 
> Jacek

Yes, no worries I'm all for correctness. It was just for the pleasure or 
the argument and because I really want to be done with these stubs :)

Thanks for the thorough reviews!
-- 
Rémi Bernon <rbernon at codeweavers.com>