Bug 110678

Created attachment 144271 [details] [review]
Patch to split assert(a && b) statements into assert(a) and assert(b) in git version of xf86-video-intel

Many thanks to Adam Jackson via Maarten Maathuis for redirecting me to submit this patch here.

This patch separates each statement of the form assert(a && b) into "assert(a)" and "assert(b)" statements.

If you will excuse the boilerplate language, here are some arguments in favor of this practice.  Thanks for considering this patch.


1. Assertion failures are often sporadic, and users who report them may
   not be in a position to efficiently narrow them down further, so it
   is important to get as much precision from each assertion failure as
   possible.

2. It is a more efficient use of developers time when a bug report
   arrives if the problem has been narrowed down that much more.  A
   new bug report may initially attract more interest, so, if the
   problem has been narrowed down that much more, it may increase the
   chance that developers may invest the time to try to resolve the
   problem, and also reduce unnecessary traffic on the developer mailing
   list about possible causes of the bug that separating the assertion
   was able to rule out.
   
3. When using a debugger to step over an assertion failure in the
   first part of the statement, the second part is still tested.

4. Providing separate likelihood hints to the compiler in the form
   of separate assert statements does not require the compiler to
   be quite as smart to recognize that it should optimize both branches,
   although I do not know if that makes a difference for any compiler
   commonly used to compile X (that is, I suspect that they are all
   smart enough to realize is that "a && b" is likely true, then "a"
   is likely true and "b" is likely true).

5. In my humble opinion, separated assertions are almost always more
   readable, sometimes eliminating parentheses, often making
   references to the same values line up on the same columns, which
   can make range checks more obvious, and sometimes changing
   multi-line conditions to separate single line conditions, which can
   be recognized separately.

   Consider, for example, how columnization in this makes this range
   check more recongizable and makes it easier to recognize in a glance
   that it is a bounds check and what the bounds are:

                assert(reg.vstride >= 0 && reg.vstride < ARRAY_SIZE(vstride_for_reg));
                        ... vs. ...

                assert(reg.vstride >= 0);
                assert(reg.vstride < ARRAY_SIZE(vstride_for_reg));

   A possible counter-argument to this might be that, in a sequence of
   assertions, can be informative to group related assertions together,
   which I think is true, but it is possible to get this other means, such
   as by using blank lines to separate express such grouping.