On 2018-02-12 22:43, Alexandre Julliard wrote:
Michael Stefaniuc mstefani@winehq.org writes:
On 02/12/2018 11:23 AM, Alexandre Julliard wrote:
Michael Stefaniuc mstefani@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