Re: [PATCH 3/4] d3drm: Initial support for storing animation keys

[Henri Verbeet]

On 4 July 2017 at 09:43, Nikolay Sivov  wrote:
> +static int d3drm_animation_cmp_min(const struct d3drm_animation_keys *keys, D3DVALUE left, D3DVALUE right, DWORD *cur)
> +{
> - if (left == right)
> - {
> -
>    while (*cur > 0 && left == keys->keys[*cur - 1].time)
> -
>        (*cur)--;
> -
>    return 0;
> - }

Does this really need to happen inside the comparison function? I.e.,
couldn't you just use a single comparison function, bsearch(), and do
the fixup for duplicates afterwards?
> +static DWORD d3drm_animation_get_index(struct d3drm_animation_keys *keys, D3DVALUE time,
> - int (*compare_func)(const struct d3drm_animation_keys *keys, D3DVALUE left, D3DVALUE right, DWORD *cur))
>
> +{
> - int min = 0, max = keys->count - 1;

unsigned/SIZE_T, right?
> +static BOOL d3drm_animation_get_range(struct d3drm_animation_keys *keys, D3DVALUE time_min, D3DVALUE time_max,
> -
>    DWORD *min, DWORD *max)
>
> +{
> - if (keys->count == 0)
> -
>    return FALSE;
> -
> - if (time_min > keys->keys[keys->count - 1].time || time_max < keys->keys[0].time)
> -
>    return FALSE;
> -
> - *min = d3drm_animation_get_index(keys, time_min, d3drm_animation_cmp_min);
> - *max = d3drm_animation_get_index(keys, time_max, d3drm_animation_cmp_max);

This is pretty minor, but note that to find *max you'd only have to
search through *min to the end, instead of through the entire range.
> - if (!key || key->dwSize != sizeof(*key))
> -
>    return E_INVALIDARG;

We usually print a WARN for cases where the application passes us
questionable data. That's certainly not a strict requirement, but on
occasion it does tend to make it slightly easier to figure out where
things start failing.
> -
>    memmove(keys->keys + index + 1, keys->keys + index, sizeof(*keys->keys) * (keys->count - index));

We'd typically write those as "&keys->keys[index + 1]", etc. That's
purely stylistic, of course.
> - if (d3drm_animation_get_range(&animation->rotate, time_min, time_max, &min, &max))
> - {
> -
>    *count += max - min + 1;
> -
> -
>    for (i = min; i <= max && keys; i++, cur++)
> -
>    {
> -
>        keys[cur].dwSize = sizeof(*keys);
> -
>        keys[cur].dwKeyType = D3DRMANIMATION_ROTATEKEY;
> -
>        keys[cur].dvTime = animation->rotate.keys[i].time;
> -
>        keys[cur].dwID = 0; /* FIXME */
> -
>        keys[cur].u.dqRotateKey = animation->rotate.keys[i].u.rotate;
> -
>    }
> - }

Minor, but note that it would probably be more convenient if
d3drm_animation_get_range() returned a pointer to
animation->rotate.keys[min] and a count.