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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 03, 2015 at 01:52:04PM -0300, Paulo Zanoni wrote:
> 2015-07-03 6:23 GMT-03:00 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.
> 
> 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...

Well I was the one who added igt_assert_cmpint, but I didn't add all the
_lte/gte/eq variants. I don't have a personal opinion about this so I'm
totally open to going back to only igt_assert_cmpint everywhere (with
cocci this is easy). But we'd need someone to push the discussion and get
acks from everyone who seems to care (you have mine already). Matt Roper
and Thomas Wood are probably the ones to poke.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx