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.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
