On 08/18/2017 04:33 PM, Philippe Mathieu-Daudé wrote:
Hi Eric,
On 08/18/2017 06:15 PM, Eric Blake wrote:
Assertions should be separate from the side effects, since in
theory, g_assert() can be disabled (in practice, we can't really
ever do that).
What about the suggestion on your "Hacks for building on gcc 7 / Fedora
26" series about avoid building without assertions?
NDEBUG doesn't affect g_assert() (only assert(), but that wasn't in use
here) - I have to double-check glib documentation to see whether
g_assert() can be crippled in a manner similar to how I know assert()
can be crippled. Ideally, we have a form that always performs side
effects exactly once, whether or not abort-on-error is enabled (assert()
does not have that, but I don't know whether glib does).
The obvious win is a more readable code.
http://lists.nongnu.org/archive/html/qemu-devel/2017-07/msg06084.html
Indeed, that's a patch proposal that I still haven't written, but it can
be done independently of this series.
Still, even if we guarantee that assertions will never be crippled for
qemu, it is bad practice to code side-effects in an assert(), because it
makes the code harder to copy-and-paste into projects that DO work with
assertions disabled, and it raises red flags in reviewers' minds of
whether it was intentional.
[And truth be told, this particular patch is not really about libqtest,
so much as something that horrified me when I was grepping for
qemu_strtoul() in order to fix the checkpatch warning I got on
libqtest's raw use of strtol() in patch 6, and which necessitated me to
write patch 5 - even though the name of the file in question is 'qtest.c']