[PATCH] include: Add and use a global heap_strdupW() helper

[Michael Stefaniuc]

On 2018-02-12 22:43, Alexandre Julliard wrote:
> Michael Stefaniuc <mstefani at winehq.org> writes:
> 
>> On 02/12/2018 11:23 AM, Alexandre Julliard wrote:
>>> Michael Stefaniuc <mstefani at winehq.org> writes:
>>> 
>>>> +static inline WCHAR *heap_strdupW(const WCHAR *str)
>>>> +{
>>>> +    WCHAR *dst;
>>>> +    SIZE_T len;
>>>> +
>>>> +    if(!str)
>>>> +        return NULL;
>>>> +
>>>> +    len = (lstrlenW(str) + 1) * sizeof(*str);
>>>> +    dst = heap_alloc(len);
>>>> +    if (dst)
>>>> +        memcpy(dst, str, len);
>>>> +
>>>> +    return dst;
>>>> +}
>>> 
>>> I'm not sure the NULL check is a good idea.
>> And I disagree with your disagreement!
>> 
>> I just sent in
>>    [PATCH] wininet: Avoid passing NULL to heap_strdupW()
>> to show how ugly that would make the code. And that's the stuff that
>> just crashed due to the tests.
>> 
>> If you want the NULL check removed for the other strdup variants,
>> especially the heap_strdupAW(), that would uglify the code even more 
>> as
>> that is used in a ton of A functions that just forward to the W 
>> functions.
> 
> The current usage is clean only because we are not checking for
> allocation failure, but that's broken. If we add proper handling, then
> the NULL checks will be needed anyway.
I assumed you want less HeapAlloc failure handling and not more!
Especially as in the strdup cases the current "return NULL" seems to be 
good enough in practice.
I don't remember to have ever seen a patch that adds extra error 
handling for that case.

So as your use case seems to be the exotic one it makes sense to move 
the extra check inside the helper and not have every caller deal with 
that.
That would require changing the signature of the helper, e.g. (I don't 
care about the name nor parameter ordering):

BOOL heap_strdup_w(WCHAR **dst, const WCHAR *src);

FALSE would be only returned if the heap_alloc() fails.

I can easily automate the transformation to the new form without adding 
any extra error handling:
     foo = heap_strdupW(bar);
would become
     heap_strdup_w(&foo, bar);


But adding the error check is left for the interested reader that cares 
about that.
That cannot be automated.
     if (heap_strdup_w(&foo, bar))
         return WHATERRORISAPPROPRIATEHERE;
I doubt that we will see many of those.


bye
       michael