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

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch