Re: [PATCHv4 2/3] wined3d: Use an rbtree for storing shaders for texture format conversion/blitting

[Martin Storsjö]

On Fri, 14 Feb 2014, Henri Verbeet wrote:
> On 13 February 2014 13:05, Martin Storsjo  wrote:
> > static HRESULT arbfp_blit_alloc(struct wined3d_device *device)
> >  {
> > - struct arbfp_blit_priv *priv;
> > device->blit_priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(struct arbfp_blit_priv));
> > if(!device->blit_priv) {
> >     ERR("Out of memory\n");
> >     return E_OUTOFMEMORY;
> > }
> > - priv = device->blit_priv;
> > -
> > - if (wine_rb_init(&priv->shaders, &wined3d_arbfp_blit_rb_functions) == -1)
> > - {
> > -
> >    ERR("Failed to initialize rbtree.\n");
> > -
> >    HeapFree(GetProcessHeap(), 0, device->blit_priv);
> > -
> >    device->blit_priv = NULL;
> > -
> >    return E_OUTOFMEMORY;
> > - }
> > - return WINED3D_OK;
> >
> > }
>
> The following is probably slightly nicer:
>
> static HRESULT arbfp_blit_alloc(struct wined3d_device *device)
> {
> - device->blit_priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
>
> sizeof(struct arbfp_blit_priv));
> - if(!device->blit_priv) {
> -
>    ERR("Out of memory\n");
> - struct arbfp_blit_priv *priv;
> -
> - if (!(priv = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*priv))))
>    return E_OUTOFMEMORY;
> -
> - if (wine_rb_init(&priv->shaders, &wined3d_arbfp_blit_rb_functions) == -1)
> - {
> -
>    ERR("Failed to initialize rbtree.\n");
> -
>    HeapFree(GetProcessHeap(), 0, priv);
> -
>    return E_FAIL;
> }
> -
> - device->blit_priv = priv;
> - return WINED3D_OK;
>
> }

That's nicer indeed, updated the patch accordingly locally.
> > - if (desc)
> > -
> >    HeapFree(GetProcessHeap(), 0, desc);
>
> There's no point in checking for NULL before HeapFree().

MSDN says "If this pointer is NULL, the behavior is undefined" about the 
lpMem parameter to HeapFree.
> Perhaps more importantly, I think it makes more sense to insert the 
> rbtree entries in arbfp_blit_set(), instead of duplicating the code 
> between gen_p8_shader() and gen_yuv_shader().

Good point, I've updated my patch accordingly.

I'll resend all three patches in a little while unless you or Stefan have 
got more comments at the moment.

// Martin