On 6 July 2015 at 08:48, Daniel Vetter <daniel@xxxxxxxx> wrote:
> 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@xxxxxxxxx>:
>> > 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@xxxxxxxxxxxxxxxxxx>:
>> >>>>
>> >>>> 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.
Chris Wilson added the ncmp parameter to igt_assert_cmpint to make the
output messages more readable, which lead to the various _lte/_eq/etc.
variants.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
