[PATCH 1/2] include: Add generic HeapAlloc() wrappers

Jacek Caban jacek at codeweavers.com
Tue Jan 23 07:25:11 CST 2018


Hello Michael,

On 01/22/2018 08:23 PM, Michael Stefaniuc wrote:
> Hello Jacek,
>
> On 01/22/2018 04:05 PM, Jacek Caban wrote:
>> On 01/21/2018 10:05 PM, Michael Stefaniuc wrote:
>>> Signed-off-by: Michael Stefaniuc <mstefani at winehq.org>
>>> ---
>>> I know that Alexandre and Jacek wanted to work on the HeapAlloc
>>> wrappers. But I don't remember if I had promised (or not) at the last
>>> WineConf to dig up my old patch and polish it up and submit it to serve
>>> as a discussion start point. Anyway, here it is with some explanations.
>> Thanks for looking at this. This looks mostly good for me. I have a few
>> suggestions to consider.
> thanks for reviewing the patch.
>
> Before I reply to your suggestions here are two considerations that were
> at the base of my proposal:
>
> Principle of the least surprise
> -------------------------------
> - Developers are already used to the current wrappers.
> - Linux developers are used to the malloc, realloc, calloc functions
> while Windows developers are used to HeapAlloc(). The function names
> will confer an expectation of what the wrappers do. If possible we
> should fulfill that expectation.
> Ease the transition
> -------------------
> - There should be no flag date for the move to the new wrappers.
> - Big churn patches should be avoided.
> - The full transition will take years, thus there will be new and old
> wrappers side by side. Those should have the same signature and the
> behavior should not differ wildly.
> - As I'll probably have to do a big part of this work you don't want me
> to run away screaming. And if I do, the code should be still in an
> acceptable consistent state.
>
>
>
>>> The _nofail variants should not "fail" memory allocations and will raise an
>>> exception on out of memory conditions.
>>> heap_alloc_nofail thus will never return NULL.
>>> heap_realloc_nofail can still return NULL but *only* when the alloc size
>>> is 0 and thus functions as heap_free. Checking the returned pointer for
>>> NULL is not needed when the size != 0. And the "assign to temp pointer
>>> to prevent mem leak on failure" workaround is never needed.
>> I think that most realloc use cases will use fallible variant (since its
>> usage usually means that the buffer may grow) and they still need
>> temporary pointer. How about using something like:
>> BOOL heap_realloc(void **mem, SIZE_T size)
>> instead? You could leave passed pointer alone on failure and set it to
>> new memory on success.
> Alexandre's requirement was a terse "sane behavior like realloc".
>
> I see your proposal has some merit.
> But why stop there? We could just free the old pointer on allocation
> failure while keeping the existing function signature.
> I didn't check but I would assume that is the common case when the temp
> pointer trick is used. That can be than simplified to just without
> leaking any memory:
> if (!(ptr = heap_realloc(ptr, size))
>     return E_OUTOFMEMORY;
>
> We can have another wrapper heap_reallocex() that implements your
> suggestion for the cases when a different cleanup is needed. Of course
> with a better name for the wrapper.
>
> Anyway, both change variants are possible from a churn point of view:
> wine$ git grep heap_realloc | wc -l
> 196

The code in your example is indeed nicer. However, I have mixed feelings
about it mostly because it would look like a familiar realloc function,
but have a very signifiant difference. This may be misleading, I
wouldn't expect such function to free memory. It's not a strong opinion
through.

>>> The _nofail variants should be used only for small allocations that are
>>> not application driven.
>>> Preferabely the allocation sizes should be known at compile time.
>>> Careful when allocating memory for COM and other objects: The main
>>> object should not be allocated with the _nofail variants. Subsequent
>>> small allocations in the same object could use the _nofail version.
>> How about having just heap_alloc as infallible and heap_calloc as
>> fallible? If you need fallible heap_alloc, you may just pass extra 1
>> argument to heap_calloc. If you need infallible heap_calloc, then you
>> know there won't be an overflow and may just multiply arguments.
> You want to change
>
> if (!(obj = heap_alloc(sizeof(*obj)))
>     return E_OUTOFMEMORY;
>
> to
>
> if (!(obj = heap_calloc(sizeof(*obj), 1))
>     return E_OUTOFMEMORY;
>
> ?
>
> Ugh, NACK.
> Quite a surprising change. Requires a flag date or big churn patches.
> The default heap_alloc() should keep its current behavior.
> That allows for a proper code review and transition to the infallible
> alloc behavior.

Sure, fine with me.

> If somebody else has a better name for the "nofail" variants I'm all
> ears. The only other name I could came up that matched the intent was
> heap_alloc_I_reviewed_the_code_and_know_what_I_am_doing(); but that's
> even longer ;)

heap_alloc_infallible_makes_allocation_trivial_so_lets_give_developer_time_to_think_about_it_while_typing()
? ;)

Thanks,
Jacek



More information about the wine-devel mailing list