Re: [Qemu-devel] [PATCH v5 02/13] qtest: Don't perform side effects inside assertion

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]