[PATCH v5 xserver 3/7] xf86Cursor: Add xf86CheckHWCursor() helper function

[Daniel Martin]

On 6 September 2016 at 14:48, Hans de Goede <hdegoede at redhat.com> wrote:
> HI,
>
>
> On 06-09-16 14:35, Daniel Martin wrote:
>>
>> Sorry for jumping in that late ...
>>
>> On 6 September 2016 at 13:31, Hans de Goede <hdegoede at redhat.com> wrote:
>>>
>>> From: Dave Airlie <airlied at redhat.com>
>>>
>>> This is a preparation patch for adding prime hw-cursor support.
>>>
>>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> ---
>>>  hw/xfree86/ramdac/xf86Cursor.c     | 11 ++---------
>>>  hw/xfree86/ramdac/xf86CursorPriv.h |  1 +
>>>  hw/xfree86/ramdac/xf86HWCurs.c     | 12 ++++++++++++
>>>  3 files changed, 15 insertions(+), 9 deletions(-)
>>>
>> ...
>>>
>>> diff --git a/hw/xfree86/ramdac/xf86HWCurs.c
>>> b/hw/xfree86/ramdac/xf86HWCurs.c
>>> index 458781c..0f6990a 100644
>>> --- a/hw/xfree86/ramdac/xf86HWCurs.c
>>> +++ b/hw/xfree86/ramdac/xf86HWCurs.c
>>> @@ -130,6 +130,18 @@ xf86ShowCursor(xf86CursorInfoPtr infoPtr)
>>>  }
>>>
>>>  Bool
>>> +xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr
>>> infoPtr)
>>> +{
>>> +    return
>>> +        (cursor->bits->argb && infoPtr->UseHWCursorARGB &&
>>> +         infoPtr->UseHWCursorARGB(pScreen, cursor)) ||
>>> +        (cursor->bits->argb == 0 &&
>>> +         cursor->bits->height <= infoPtr->MaxHeight &&
>>> +         cursor->bits->width <= infoPtr->MaxWidth &&
>>> +         (!infoPtr->UseHWCursor || infoPtr->UseHWCursor(pScreen,
>>> cursor)));
>>> +}
>>
>>
>> You just moved the code, but do you mind splitting the if-clause to
>> make it more pleasant for the eyes? I.e.
>>
>>     if (cursor->bits->argb) {
>>         if (infoPtr->UseHWCursorARGB)
>>             return infoPtr->UseHWCursorARGB(pScreen, cursor);
>>     } else
>>     if (cursor->bits->width <= infoPtr->MaxWidth &&
>>         cursor->bits->height <= infoPtr->MaxHeight) {
>>         if (infoPtr->UseHWCursor)
>>             return infoPtr->UseHWCursor(pScreen, cursor)
>>         else
>>             return TRUE;
>>     }
>>
>> As my comment came late and is beautifying only, feel free to ignore it.
>
>
> I'm not sure I find that more readable. Point of proof:

I had to look twice to untangle it. If you don't like to change it,
there's no objection to keep it as is.

> You're missing a return FALSE when cursor->bits->argb != NULL and
> infoPtr->UseHWCursorARGB == NULL

Yes, I didn't pasted it. That would be the default fall through of the function.