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

[Paulo Zanoni]

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.


>>
>>       rc = read(sink_crc.fd, crc.data, SINK_CRC_SIZE);
>>       errno_ = errno;
>> @@ -1184,7 +1184,7 @@ static bool fbc_supported_on_chipset(void)
>>  static void setup_fbc(void)
>>  {
>>       fbc.fd = igt_debugfs_open("i915_fbc_status", O_RDONLY);
>> -     igt_assert(fbc.fd >= 0);
>> +     igt_assert_lte(0, fbc.fd);
>>
>>       if (!fbc_supported_on_chipset()) {
>>               igt_info("Can't test FBC: not supported on this chipset\n");
>> @@ -1220,7 +1220,7 @@ static void setup_psr(void)
>>       }
>>
>>       psr.fd = igt_debugfs_open("i915_edp_psr_status", O_RDONLY);
>> -     igt_assert(psr.fd >= 0);
>> +     igt_assert_lte(0, psr.fd);
>>
>>       if (!psr_sink_has_support()) {
>>               igt_info("Can't test PSR: not supported by sink.\n");
>> @@ -1426,13 +1426,13 @@ static void set_cursor_for_test(const struct test_mode *t,
>>       fill_fb_region(&params->cursor, 0xFF0000FF);
>>
>>       rc = drmModeMoveCursor(drm.fd, params->crtc_id, 0, 0);
>> -     igt_assert(rc == 0);
>> +     igt_assert_eq(rc, 0);
>
> As a general comment, not on your patch, this assert doesn't provide
> anywhere near the right information. rc here is either -1 or 0, it's the
> errno that's interesting (fortunately also printed by the assert), but
> it is igt_assert_eq(drmModeModeCursor(drm.fd, params->crtc_id, 0, 0), 0);
> that provides the most useful immediate debugging aid.
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni