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

Submitted by Adel Gadllah on Feb. 26, 2014, 11:27 p.m.

Details

Message ID 1393457229-6444-3-git-send-email-adel.gadllah@gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Adel Gadllah Feb. 26, 2014, 11:27 p.m.
Move the pdraw != NULL check out so that they don't
have to be duplicated.

Signed-off-by: Adel Gadllah <adel.gadllah@gmail.com>
Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
---
 src/glx/glx_pbuffer.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

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
          _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);
          }

Comments

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@gmail.com>
> Reviewed-by: Ian Romanick <ian.d.romanick@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.

>           _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);
>           }
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@gmail.com>
>> Reviewed-by: Ian Romanick <ian.d.romanick@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.

>>            _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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
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@gmail.com>
>>> Reviewed-by: Ian Romanick <ian.d.romanick@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@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
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@gmail.com>
>>>> Reviewed-by: Ian Romanick <ian.d.romanick@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.