color_match in dlls/d3d8/tests/visual.c may not do what one would think

[Henri Verbeet]

On Mon, 5 Nov 2018 at 01:26, Gerald Pfeifer <gerald at pfeifer.com> wrote:
>
> Hi Henri, hi everyone,
>
> earlier this year via
>
>    commit 2f6fbdec8caee311e2fb85b2ddc9fbd8a9afaea5
>    Author: H. Verbeet <hverbeet at gmail.com>
>    Date:   Sat May 24 10:33:36 2008 +0200
>
>        d3d8: Test our texop implementation.
>
> we go the following function in dlls/d3d8/tests/visual.c:
>
>    static BOOL color_match(D3DCOLOR c1, D3DCOLOR c2, BYTE max_diff)
>    {
>        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
>        c1 >>= 8; c2 >>= 8;
>        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
>        c1 >>= 8; c2 >>= 8;
>        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
>        c1 >>= 8; c2 >>= 8;
>        if (abs((c1 & 0xff) - (c2 & 0xff)) > max_diff) return FALSE;
>        return TRUE;
>    }
>
> Intuitively it's clear what this does, however there's a little
> challenge: c1 and c2 are unsigned integers (D3DCOLOR being DWORD
> being unsigned long), and so are c1 & 0xff and c2 & 0xff.
>
> Substraction of two unsigned integer types is done in unsigned
> math, so we may be seeing wrapping, but never a negative number
> as a result -- and abs() is a noop.
>
As Ken pointed out, the code has been in the tree a little longer than
this year. As for the issue you point out, it's true that the function
assumes sizeof(DWORD) == sizeof(int), and a two's complement system.
I.e., in two's complement 0u - 1u == ~0u == -1, etc. I don't think
those are unreasonable assumptions. Perhaps the function has room for
improvement, but I'd prefer to not simply sprinkle some casts on top.