Hi!
2024-12-06 at 21:13, David Laight wrote:
> From: 'Andy Shevchenko'
>> Sent: 06 December 2024 15:20
> ...
>> ...
>>
>>>> * If only one of the rescaler elements or the schan scale is
>>>> * negative, the combined scale is negative.
>>>> */
>>>> - if (neg ^ ((rescale->numerator < 0) ^ (rescale->denominator < 0))) {
>>>> + if (neg != (rescale->numerator < 0 || rescale->denominator < 0)) {
>>>
>>> That is wrong, the || would also need to be !=.
>>
>> Why do you think so? Maybe it's comment(s) that is(are) wrong?
>
> The old code certainly negates for each of them - so you can get the double
> and triple negative cases.
Indeed. And for me, xor is the natural operator for getting to such
"oddness" when three or more values needs to be considered. So, I
prefer to keep the code as is since a ^ b ^ c is nice and succinct,
while anything you try to write using != is going to be convoluted
when you have three or more values.
> So believe the code not the comment.
I claim that the comment is correct. Or at least not incorrect. It might
not be complete, but what it does state holds. It does not spell out that
the combined scale is positive if both of the rescaler elements and the
schan scale are positive. It does not spell out that the combined scale
is negative if all three are negative. What it does give you though, is
a hint that the whole if () is all about the sign of the combined scale.
But yes, the comment could be improved.
I expect a fail from this test in iio-test-rescale.c with the new code
{
.name = "negative IIO_VAL_INT_PLUS_NANO, 3 negative",
.numerator = -1000000,
.denominator = -8060,
.schan_scale_type = IIO_VAL_INT_PLUS_NANO,
.schan_val = -10,
.schan_val2 = 123456,
.expected = "-1240.710106203",
},
but the 0-day has been silent. I wonder why? Does it not actually
run the tests?
There could also be some more tests to try make sure the logic doesn't
regress. The first of these should also fail with this patch, while
the second should be ok.
{
.name = "positive IIO_VAL_INT_PLUS_MICRO, 2 negative",
.numerator = -1,
.denominator = -2,
.schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
.schan_val = 5,
.schan_val2 = 1234,
.expected = "2.500617",
},
{
.name = "positive IIO_VAL_INT_PLUS_MICRO, 2 negative",
.numerator = 1,
.denominator = -2,
.schan_scale_type = IIO_VAL_INT_PLUS_MICRO,
.schan_val = -5,
.schan_val2 = 1234,
.expected = "2.500617",
},
Cheers,
Peter
