From: Michael Stefaniuc Subject: Re: [PATCH] include: Add and use a global heap_strdupW() helper Message-Id: <9c73a913-55de-9dbb-bddd-cd1c7aeb70e2@winehq.org> Date: Tue, 13 Feb 2018 21:40:26 +0100 In-Reply-To: <87a7wdb4lk.fsf@winehq.org> References: <20180209204421.12241-1-mstefani@winehq.org> <87o9kuo6ac.fsf@winehq.org> <87vaf1riir.fsf@winehq.org> <87a7wdb4lk.fsf@winehq.org> On 02/13/2018 10:50 AM, Alexandre Julliard wrote: > Michael Stefaniuc 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