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

[Michael Stefaniuc]

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 _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.

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 ;)


bye
	michael