Proposed addition to CodingStyle web page about assert(a && b)
Adam Richter
adamrichter4 at gmail.com
Sat May 4 22:47:31 UTC 2019
Hi, everyone.
I would like to propose that whoever has the ability to edit the web
page add a line like the following to
https://www.x.org/wiki/CodingStyle/ :
- Separate assert(a && b) into assert(a) and assert(b).
I can think of several potential advantages of separating logical conjunctions
in assertions:
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. It's often more readable, sometimes eliminating parentheses or
changing multi-line conditions to separate single line conditions.
4. When using a debugger to step over an assertion failure in the
first part of the statement, the second part is still tested.
5. 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).
With that said, I have noticed at least one clever reasonable exception to this,
which is an assert statement like this one in
hw/xfree86/drivers/modesetting/drmmode_display.c, where the second condition
is just a string that that the develop wants to have included in that
failure message:
assert(num_infos <= 32 && "update return type");
I would welcome some way to state that in the coding style that is
nearly as concise as the way the other lines in that web page are
written.
As a bit of background, I want to mention that I went on a mostly
successful similar campaign for BUG_ON(a || b) in the Linux kernel
over the course of a week or so about a decade ago but never bothered
to ask for a change in the coding guidelines there, and a few days ago
I noticed that some statements of that form have crept back in. So,
learning from that experience, I figure I should at least ask to add
this to the coding style document.
To avoid duplication of effort, I will also mention that I have
already submitted a change on gitlab to separate such assertions in
the core X server and am about to submit a similar patch for
xf86-video-intel.
Thanks in advance for any input on this.
Adam
More information about the xorg-devel
mailing list