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!