[PATCH xserver (v4) 03/10] Make Await SyncTrigger functions generic

[Keith Packard]

On Mon, 6 Dec 2010 14:53:17 -0800, James Jones <jajones at nvidia.com> wrote:
> Update all the functions dealing with Await
> sync triggers handle generic sync objects
> instead of just counters.  This will
> facilitate code sharing between the counter
> sync waits and the fence sync waits.

> -    if (IsSystemCounter(pTrigger->pCounter))
> -	SyncComputeBracketValues(pTrigger->pCounter);
> +    if (SYNC_COUNTER == pTrigger->pSync->type)
> +    {
> +	pCounter = (SyncCounter *)pTrigger->pSync;
> +
> +	if (IsSystemCounter(pCounter))
> +	    SyncComputeBracketValues(pCounter);
> +    }

Seeing changes like this, it's tempting to use a union somewhere so you
could avoid all of these casts.

> @@ -188,69 +200,91 @@ SyncAddTriggerToCounter(SyncTrigger *pTrigger)
>  static Bool
>  SyncCheckTriggerPositiveComparison(SyncTrigger *pTrigger, CARD64 oldval)
>  {
> -    return (pTrigger->pCounter == NULL ||
> -	    XSyncValueGreaterOrEqual(pTrigger->pCounter->value,
> -				     pTrigger->test_value));
> +    SyncCounter *pCounter;
> +
> +    assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));
> +    pCounter = (SyncCounter *)pTrigger->pSync;
> +
> +    return (pCounter == NULL ||
> +	    XSyncValueGreaterOrEqual(pCounter->value, pTrigger->test_value));
>  }

Do we really need asserts in these functions? An error message and the
obvious return might be kinder to the user in case of errors elsewhere
in the server.

> +    SyncCounter *pCounter;
> +
> +    assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));
> +
> +    pCounter = (SyncCounter *)pTrigger->pSync;

Btw, I notice you consistently place of the constant on the left side of
the == operator. This isn't common practice in the X server, but I could
grow to like it. Obviously avoids any accidental assignments.

> +    assert(!pTrigger->pSync || (SYNC_COUNTER == pTrigger->pSync->type));

Again, we probably don't want an assert here.

(this code looks really nice in general)

Reviewed-by: Keith Packard <keithp at keithp.com>

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20101206/fb94f9f7/attachment.pgp>