[Mesa-dev] [PATCHv4 2/3] glx_pbuffer: Refactor GetDrawableAttribute

[Jason Wood]

On 02/26/2014 06:55 PM, Ian Romanick wrote:
> On 02/26/2014 05:22 PM, Jason Wood wrote:
>> On 02/26/2014 04:27 PM, Adel Gadllah wrote:
>>> Move the pdraw != NULL check out so that they don't
>>> have to be duplicated.
>>>
>>> Signed-off-by: Adel Gadllah <adel.gadllah at gmail.com>
>>> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>>> ---
>>>   src/glx/glx_pbuffer.c | 11 ++++++-----
>>>   1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/src/glx/glx_pbuffer.c b/src/glx/glx_pbuffer.c
>>> index 411d6e5..978730c 100644
>>> --- a/src/glx/glx_pbuffer.c
>>> +++ b/src/glx/glx_pbuffer.c
>>> @@ -350,6 +350,9 @@ GetDrawableAttribute(Display * dpy, GLXDrawable
>>> drawable,
>>>            _XEatData(dpy, length);
>>>         }
>>>         else {
>>> +#if defined(GLX_DIRECT_RENDERING) && !defined(GLX_USE_APPLEGL)
>>> +         __GLXDRIdrawable *pdraw = GetGLXDRIDrawable(dpy, drawable);
>>> +#endif
>> What is the point of moving the declaration of pdraw into a separate
>> ifdef from the only one it is used in?  It seems to me that this change
>> only serves to make the code less readable.
>
> This was my recommendation.  It eliminates needing to add yet another
> level of indentation below... especially with patch 3/3 on top of it.

That is fair.  I should have taken a closer look at 3/3 before commenting. 

However, patch 3/3 again moves the declaration for pdraw -- to the top
of the function.  Why move the declaration twice?  No need for the extra
code churn...



>
>>>            _XRead(dpy, (char *) data, length * sizeof(CARD32));
>>>
>>>            /* Search the set of returned attributes for the
>>> attribute requested by
>>> @@ -363,13 +366,11 @@ GetDrawableAttribute(Display * dpy,
>>> GLXDrawable drawable,
>>>            }
>>>
>>>   #if defined(GLX_DIRECT_RENDERING) && !defined(GLX_USE_APPLEGL)
>>> -         {
>>> -            __GLXDRIdrawable *pdraw = GetGLXDRIDrawable(dpy,
>>> drawable);
>>> -
>>> -            if (pdraw != NULL && !pdraw->textureTarget)
>>> +         if (pdraw != NULL) {
>>> +            if (!pdraw->textureTarget)
>>>                  pdraw->textureTarget =
>>>                     determineTextureTarget((const int *) data,
>>> num_attributes);
>>> -            if (pdraw != NULL && !pdraw->textureFormat)
>>> +            if (!pdraw->textureFormat)
>>>                  pdraw->textureFormat =
>>>                     determineTextureFormat((const int *) data,
>>> num_attributes);
>>>            }
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>