[PATCH] dix: add valuator_mask_fetch_double()

[Chase Douglas]

On 09/30/2011 04:40 PM, Daniel Stone wrote:
> On 1 October 2011 00:32, Chase Douglas <chase.douglas at canonical.com> wrote:
>> On 09/30/2011 04:07 PM, Daniel Stone wrote:
>>> e.g.:
>>> double valuator_mask_fetch_double(struct valuator_mask *mask, int index)
>>> {
>>>     if (valuator_mask_isset(mask, index)
>>>         return valuator_mask_get_double(mask, index);
>>>     else
>>>         return 0.0;
>>> }
>>
>> Two issues:
>>
>> * Can we be sure that 0.0 will never be a valid value in the mask? My
>> hunch is no.
> 
> No, but people who need to differentiate between 0.0 and NaN can use
> valuator_mask_isset, just as before.

That sounds very error prone. It would have to be well documented, and
even then people might get caught by copy/paste. I wouldn't be in favor
of this approach.

>> * If we did this, we should do the same for normal int values. I can
>> guarantee that 0 is a valid value in some cases, which makes this
>> impossible. Ergo, we shouldn't do this for doubles either.
>>
>> If you really want to clean things up more, you could do:
>>
>> double valuator_mask_choose_double(struct valuator_mask *mask, int
>>                                   index, double value_if_unset)
>> {
>>    if (valuator_mask_isset(mask, index)
>>        return valuator_mask_get_double(mask, index);
>>    else
>>        return value_if_unset;
>> }
>>
>> (I don't really like the name, but couldn't come up with a better one in
>> 2 minutes.)
> 
> Whichever value you pick will still be valid as a ValuatorMask value.
> 
>> This would condense code blocks from:
>>
>> if (!valuator_mask_fetch_double(mask, i, &value))
>>    value = prev_value;
>>
>> to:
>>
>> value = valuator_mask_choose_double(mask, i, prev_value);
> 
> Still though, it's just changing it from:
> if (valuator_mask_isset(&mask, i))
>     value = valuator_mask_get_double(&mask, i);
> else
>     do_something_else();
> to:
> value = valuator_mask_fetch_double(&mask, i, SENTINEL_CONSTANT);
> if (value == SENTINEL_CONSTANT)
>     do_something_else();
> 
> Which doesn't seem massively valuable to me.

The point is to use a meaningful value, not a sentinal. In the code
there are places where you need to use the current value of a valuator,
whether that valuator is in the mask or saved off somewhere. For
example, the transformation input matrix calculation can only be done
with the current values of both X and Y, even if only one of them is set
in the valuator mask. In this case, one would do (pseudo-code):

transform(transform_matrix, valuator_mask_choose(mask, 0, prev_x),
          valuator_mask_choose(mask, 1, prev_y);

Instead of:

int cur_x;
int cur_y;
if (!valuator_mask_fetch(mask, 0, &cur_x))
    cur_x = prev_x;
if (!valuator_mask_fetch(mask, 0, &cur_y))
    cur_y = prev_y;
transform(transform_matrix, cur_x, cur_y);

(You can condense further and remove two lines if you assume that the
passed in pointer won't be modified by the fetch functions if the mask
isn't set, but still...)

>> It would also satisfy the ability to use it in places where you don't
>> really need a variable:
>>
>> if (valuator_mask_choose_double(mask, i, prev_value) > 0) { ... }
>
> This could just be:
> if (valuator_mask_get_double(&mask, i) != prev_value) { ... }
> ?

This:

if (valuator_mask_choose_double(mask, i, prev_value) > 0) { ... }

is for checking whether the current value of the valuator meets a
certain condition. Perhaps it would be more clear to use a different
condition:

if (valuator_mask_choose_double(mask, i, prev_value) < -5.6) { ... }

This:

if (valuator_mask_get_double(&mask, i) != prev_value) { ... }

would always be true (in theory). The value shouldn't be set in the mask
if it hasn't changed. I'm not real sure what you meant to point out with
this :).

-- Chase