[Spice-devel] [PATCH v2 04/13] Rephrase assertion section

[Christophe de Dinechin]

> On 8 Feb 2018, at 10:35, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> 
>> 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”.

OK. So nobody really knows how to phrase this then ;-)

What about this:

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.

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.

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.


I’m handwaving a little too much to my taste here, but I’m afraid than anything more precise will lead to another round of debate ;-)

> 
> Frediano