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

[Jacek Caban]

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