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'] -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
