Re: [PATCH 3/5] usp10: Use heap_alloc_zero() instead of HeapAlloc() with HEAP_ZERO_MEMORY.

[Sebastian Lackner]

On 09.03.2017 10:37, Henri Verbeet wrote:
> -static inline BOOL heap_free(void *mem)
> -{
> - return HeapFree(GetProcessHeap(), 0, mem);
>
> -}

[...]
> +static inline void heap_free(void *mem)
> +{
> - HeapFree(GetProcessHeap(), 0, mem);
>
> +}

I'm not sure if Francois is happy with this change. In other dlls, he
has made the exact opposite change in his effort to standardize those
functions.

[PATCH 3/5] usp10: Use heap_alloc_zero() instead of HeapAlloc() with HEAP_ZERO_MEMORY.

[Henri Verbeet]

On 9 March 2017 at 15:51, Sebastian Lackner  wrote:
> On 09.03.2017 10:37, Henri Verbeet wrote:
> > -static inline BOOL heap_free(void *mem)
> > -{
> > - return HeapFree(GetProcessHeap(), 0, mem);
> >
> > -}
>
> [...]
> > +static inline void heap_free(void *mem)
> > +{
> > - HeapFree(GetProcessHeap(), 0, mem);
> >
> > +}
>
> I'm not sure if Francois is happy with this change. In other dlls, he
> has made the exact opposite change in his effort to standardize those
> functions.

I wasn't aware of that. While I don't care particularly much if it's
there or not, I'd be somewhat surprised if there are legitimate uses
of the return value. At least inside usp10 it's always ignored.

[PATCH 3/5] usp10: Use heap_alloc_zero() instead of HeapAlloc() with HEAP_ZERO_MEMORY.

[Aric Stewart]

On 3/9/17 9:02 AM, Henri Verbeet wrote:
> On 9 March 2017 at 15:51, Sebastian Lackner  wrote:
> > On 09.03.2017 10:37, Henri Verbeet wrote:
> > > -static inline BOOL heap_free(void *mem)
> > > -{
> > > - return HeapFree(GetProcessHeap(), 0, mem);
> > >
> > > -}
> >
> > [...]
> > > +static inline void heap_free(void *mem)
> > > +{
> > > - HeapFree(GetProcessHeap(), 0, mem);
> > >
> > > +}
> >
> > I'm not sure if Francois is happy with this change. In other dlls, he
> > has made the exact opposite change in his effort to standardize those
> > functions.
>
> I wasn't aware of that. While I don't care particularly much if it's
> there or not, I'd be somewhat surprised if there are legitimate uses
> of the return value. At least inside usp10 it's always ignored.

I personally have no passion either way either. Having it be void vs BOOL is all the same to me.
Francois? Do you care, if so then I think your caring will win. :)

To not slow down my review I will ignore this issue and look at the rest. Since I don't care my resulting sign-offs will reflect that.

-aric

[PATCH 3/5] usp10: Use heap_alloc_zero() instead of HeapAlloc() with HEAP_ZERO_MEMORY.

[Francois Gouget]

On Fri, 10 Mar 2017, Aric Stewart wrote:
[...]
> I personally have no passion either way either. Having it be void vs 
> BOOL is all the same to me. Francois? Do you care, if so then I think 
> your caring will win. :)

I don't really care either way either. It would just be better for it to 
be the same everywhere so that later changes to that set of functions 
can be copy/pasted everywhere without issue.

Personally I think it would be even better to move them to something 
like include/wine/heap.h. I don't know if Alexandre would agree though.

Based on what Michael told me on IRC Alexandre would like changes to be 
made to these functions but I don't know if it impacts heap_free(). For 
instance for heap_alloc() and heap_realloc() he would probably like 
something like this:

static inline void * __WINE_ALLOC_SIZE(1) heap_alloc(size_t size)
{
    if (!size) return NULL;
    return HeapAlloc(GetProcessHeap(), 0, size);
}

static inline void* __WINE_ALLOC_SIZE(2) heap_realloc(void *mem, size_t size)
{
    if (!size)
    {
        heap_free(mem);
        return NULL;
    }
    if (!mem) return heap_alloc(size);
    return HeapReAlloc(GetProcessHeap(), 0, mem, size);
}

Then there's the string heap functions but I did not really look at 
those.

I think his idea is also to eliminate calls to heap_xxx() for small 
allocations (like a simple structure) where possible. Overall, doing 
this kind of change will require checking the call sites to make sure 
they are compatible with that and that they take advantage of them. This 
type of checks is probably more a Michael type of thing though ;-)