[Spice-devel] [PATCH v2 04/13] 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 | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 72ed2ef7..61cb0701 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -82,7 +82,11 @@ Comments that are prefixed with `FIXME` describe a bug
> that need to be fixed. Ge
>  ASSERT
>  ------
>  
> -Use it freely. ASSERT helps testing function arguments and function results
> validity.  ASSERT helps detecting bugs and improve code readability and
> stability.
> +Use assertions liberally. They helps testing function arguments and function
> results validity. Assertions helps detecting bugs and improve code
> readability and stability.
> +
> +Several form of assertion exist, notably:
> +- spice_assert which should be preferred for any assertion related to SPICE
> itself
> +- glib asserts (many forms) which should be preferred for any assertion
> related to the use of glib
>  
>  sizeof

Yes, this section looks... weird.
I think there is a problem here and this entire section is entirely broken
now.
To sum up: we don't use ASSERT! At all. But we use some kind of asserts!
Yes, sounds confusing.
But you have to consider the long story of this project (which unfortunately
I don't know entirely by myself!). Maybe I'm a bit wrong but the project was
in C++ using a lot of Windows knowledge and styles. This is a lot lost in
the current code is is hard to see but I was also a Windows developer for
quite some time (I don't regret it) and I can still note the smell of it!

What's specifically "ASSERT" (not assert, not asserts) ?
ASSERT is a Windows macro that aborts the code if the condition is false.
It helps detecting the bugs but this macro is NEVER compiled in release
code (Windows rules!). The difference seems small but is way different/
Is not used for runtime checks that can happen as this would probably
causes crashes on release. Is used for checks that should never happen
to help developers testing with debugging versions (on Windows you test
with debugg versions). How can I say that? There are some places where
this macro is too aggressive, where should be mathematically impossible
to reach. For instance in common/ring.h they are used to check
internal states at every low level operation (in functions that are
inlined). I just think in the history of code ASSERT were changed to
spice_assert. Other indications (looks like to be a paleontologist) are
in code like common/quic.c (notably encode function) which are basically
testing is a 32 bit integer has more that 32 bits!

In our code we never disable our assert style asserts (either spice_assert
or g_assert) which make them suitable for runtime checks but a bit
overwhelming to check the "obvious".

Frediano