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

[Dave Gordon <david.s.gordon@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@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.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx