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

[Christophe de Dinechin]

> On 12 Feb 2018, at 09:21, 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 | 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.

I’m sorry, I can’t make sense of this sentence. I believe you mean that the recommendation is too strong (not stronger) because we never disable them?

Do you have any performance numbers on the extra cost of assertions?


> 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).

Yes. This kind of code is the obvious result of being a bit confused on the usage of assertions.

That specific example is somewhat ridiculous, by the way. The two tests are one machine instruction each, I doubt they are even noticeable on any performance benchmark.

ENABLE_EXTRA_CHECKS is also used for additional checks that are somewhat expensive at run-time, like in display_channel_finalize. OK, but then why not make that a run-time flag instead of a compile-time flag? Unless the additional test code makes your code larger by 50%, but I doubt this is the case here…

> 
>> +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.

I mention in the text that we don’t disable them in practice.

But why should that NEVER be done? (in uppercase, no less)? If some distro has a policy of building with assertions disabled, that’s their choice.

> 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).

Check what? That G_DISABLE_ASSERT is not set?

> 
>> +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