[Intel-gfx] [PATCH 2/2 i-g-t] lib/igt.cocci: Add 64-bit and float compare functions

[Paulo Zanoni]

2015-07-03 6:23 GMT-03:00 Dave Gordon <david.s.gordon at intel.com>:
> On 01/07/15 14:02, Daniel Vetter wrote:
>>
>> On Tue, Jun 30, 2015 at 11:14:54AM -0300, Paulo Zanoni wrote:
>>>
>>> 2015-06-30 10:54 GMT-03:00 Chris Wilson <chris at chris-wilson.co.uk>:
>>>>
>>>> On Tue, Jun 30, 2015 at 02:41:09PM +0100, Michel Thierry wrote:
>>>>>
>>>>> @@ -1109,7 +1109,7 @@ static void setup_sink_crc(void)
>>>>>        set_mode_for_params(&prim_mode_params);
>>>>>
>>>>>        sink_crc.fd = igt_debugfs_open("i915_sink_crc_eDP1", O_RDONLY);
>>>>> -     igt_assert(sink_crc.fd >= 0);
>>>>> +     igt_assert_lte(0, sink_crc.fd);
>>>>> This one is wrong, and similar transformations.
>>>
>>>
>>> Maybe I'm not intelligent enough, but I _really_ think these
>>> inequality comparison macros are very hard to read, and the value they
>>> add does not compensate the readability problem they bring, especially
>>> since, as you pointed, in a lot of cases, the errno is what's
>>> important. I'd love to _not_ have that on IGT. The fact that you and
>>> Michel are discussing whether the macro is correct or not kinda proves
>>> my point on readability. I don't really want to check which one of you
>>> is correct because it's going to take some time reading the macro
>>> definition, and I've done it before and didn't like it. Reading the
>>> plain original assertion is always easy and instantaneous.
>>>
>>> Also, most of the assertions on IGT are "just in case" assertions that
>>> should probably never happen. I'm in favor of the idea that we should
>>> only "instrument" the important assertions that are likely to fail,
>>> while all the others should just be readable.
>>
>>
>> Imo igt_assert_cmpint was definitely useful for all the "did the right
>> value land" testcase. Many of those run in a loop and it's really useful
>> to see what the expected vs. real value is imo. It has gotten a bit out of
>> hand though, and some of the igt.cocci transforms that have been added
>> where plain wrong.
>>
>> But ignoring those hiccups I still think this is somewhat useful.
>> -Daniel
>
>
> At another company where we were trying to do pretty much this, we defined
> the assert-comparison macro to take the comparison operator as one of the
> arguments, thus not destroying readability quite as much:
>
> thus    assert(a >= b);         was transformed to
>
>         insist(a, >=, b);
>
> So the order of operands and the specific comparator remain clearly visible,
> rather than being interchanged or logically inverted, but the macro can
> still report both the expected and actual values, and the text of the
> expressions used for each of them and the comparator.

I like the idea, so I decided to look at how to implement that. I
discovered that igt_assert_cmpint() used to be exactly what you
described. Later we added the ncmp argument and started favoring the
usage of _lte everywhere...

>
> .Dave.



-- 
Paulo Zanoni