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

[Adel Gadllah]

Am 27.02.2014 03:14, schrieb 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...
>
>
Well I could move it to the top in the second patch as well but the thing is it does not make much sense to move it to 
the top while being only used in the else branch ...

Patch 3/3 moves it to the top because it does not have to do the round trip (everything can be done at the client side).

So its not really "extra code churn" but changes that more or less make sense in isolation.