On 02/13/2018 10:50 AM, Alexandre Julliard wrote:
Michael Stefaniuc mstefani@mykolab.com writes:
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.
You could argue that strdup() should be treated as a nofail function, but then you should remove the second null check and let it crash on the memcpy, or use HEAP_GENERATE_EXCEPTIONS. I'd prefer to have an explicit xstrdup() function for this though, and have the regular strdup() require error handling
I don't mind a xstrdup() variant, remains to be seen how useful it would be.
I consider the current strdup() behavior to be superior to what you propose as it is generic: - One can still check for alloc errors iff it is needed. - Or let subsequent code deal with the NULL pointer. This seems to be the default and seems to work pretty well in practice.
The ship has sailed anyway: wine$ git grep -i -P 'strdup(\w+)?\s*(' origin dlls/ programs/ | wc -l 1606
Not all are call sites but even 1500 cases are way too many to figure out manually what error to return. And how many of those will be a "Needs tests"? A ton of work without a clear benefit.
As the strdup() work is controversial I'll skip making those generic.
bye michael