[Mesa-dev] [PATCH 1/8] svga: eliminate unneeded gotos in svga_validate_surface_view()

[Brian Paul]

On 09/23/2016 03:28 PM, Charmaine Lee wrote:
>
>
>> From: Brian Paul <brianp at vmware.com>
>> Sent: Friday, September 23, 2016 8:48 AM
>> To: mesa-dev at lists.freedesktop.org
>> Cc: Charmaine Lee
>> Subject: [PATCH 1/8] svga: eliminate unneeded gotos in svga_validate_surface_view()
>
>> ---
>> src/gallium/drivers/svga/svga_surface.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>
>> diff --git a/src/gallium/drivers/svga/svga_surface.c b/src/gallium/drivers/svga/svga_surface.c
>> index 7cc7ef1..91bd4ca 100644
>> --- a/src/gallium/drivers/svga/svga_surface.c
>> +++ b/src/gallium/drivers/svga/svga_surface.c
>> @@ -426,14 +426,12 @@ svga_validate_surface_view(struct svga_context *svga, struct >svga_surface *s)
>>                    "same resource used in shaderResource and renderTarget 0x%x\n",
>>                    s->handle);
>>           s = create_backed_surface_view(svga, s);
>> -         if (!s)
>> -            goto done;
>> -
>> +         /* s may be null here if the function failed */
>>           break;
>>        }
>>     }
>
>> -   if (s->view_id == SVGA3D_INVALID_ID) {
>> +   if (s && s->view_id == SVGA3D_INVALID_ID) {
>>        SVGA3dResourceType resType;
>>        SVGA3dRenderTargetViewDesc desc;
>
>> @@ -478,11 +476,10 @@ svga_validate_surface_view(struct svga_context *svga, struct >svga_surface *s)
>>        if (ret != PIPE_OK) {
>>           util_bitmask_clear(svga->surface_view_id_bm, s->view_id);
>>           s->view_id = SVGA3D_INVALID_ID;
>> -         goto done;
>> +         s = NULL;
>>        }
>>     }
>
>> -done:
>>     SVGA_STATS_TIME_POP(svga_sws(svga));
>
>>     return &s->base;
>
> We want to return NULL when we fail to create RenderTargetView.

We do, but it is subtle.  "return &s->base" is a pointer arithmetic 
expression which does not actually dereference the pointer.  It would 
evaluate to NULL+offset(base) where offset(base)=0 in this case.

I guess I could change it to 'return s ? &s->base : NULL;' to be clear. 
  It would also ensure the right result if someone inadvertently moved 
the base member further down in the structure.

-Brian