[Spice-devel] [PATCH v3 04/11] Rephrase assertion section

[Frediano Ziglio]

> 
> From: Christophe de Dinechin <dinechin at redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin at redhat.com>
> ---
>  docs/spice_style.txt | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 13032df6..eef4880f 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -99,10 +99,19 @@ FIXME and TODO
>  
>  Comments that are prefixed with `FIXME` describe a bug that need to be
>  fixed. Generally, it is not allowed to commit new code having `FIXME`
>  comment. Committing  `FIXME` is allowed only for existing bugs. Comments
>  that are prefixed with `TODO` describe further features, optimization or
>  code improvements, and they are allowed to be committed along with the
>  relevant code.
>  
> -ASSERT
> -------
> +Assertions
> +----------
> +
> +Use assertions liberally. Assertions help testing function arguments and
> function results validity. As a result, they make it easier to detect bugs.
> Also, by expressing the intent of the developer, they make the code easier
> to read.
> +

I come from the Windows world and I found here in our case the "Use assertions
liberally" a bit stronger if they are always used. Kind of always using
address sanitizer compiled in even in production. Recently in spice-server
we added code like:

    if (ENABLE_EXTRA_CHECKS) {
        spice_assert(dev->hdr_pos >= sizeof(StreamDevHeader));
        spice_assert(dev->hdr.type == STREAM_TYPE_FORMAT);
    }

the ENABLE_EXTRA_CHECKS is always true/false and is a compiled in constant
and is true only if explicitly enabled so the compiler will strip the entire
checks if not enabled (but still checking for syntax).

> +Several form of assertion exist. SPICE does not use the language `assert`
> much, but instead relies on the following variants:
> +
> +- `spice_assert`, defined in `common/log.h`, is never disabled at run-time
> and prints an error if the assertion fails.
> +
> +- `g_assert` and other variants defined in glib such as `g_assert_null`, can
> be diabled by setting `G_DISABLE_ASSERT` (which is never done in practice
> for SPICE code), and emits a message through the g_assertion_message
> function and its variants.
> +

typo: "diabled" -> "disabled"
No, our code as far as I remember should NEVER be compiled and used with
G_DISABLE_ASSERT enabled. Or maybe I'm confused by G_DISABLE_CHECKS ?
OT: maybe we should check this in the code (I think better place is common/log.c).

> +The guidelines for selecting one or the other are not very clear from the
> existing code. The `spice_assert` variant may be preferred for SPICE-only
> code, as it makes it clearer that this assertion is coming from SPICE. The
> `g_assert` and variants may be preferred for assertions related to the use
> of glib.
>  
> -Use it freely. ASSERT helps testing function arguments and function results
> validity.  ASSERT helps detecting bugs and improve code readability and
> stability.
>  
>  sizeof
>  ------

Frediano