[v4,04/12] Rephrase assertion section

Submitted by Christophe de Dinechin on Feb. 15, 2018, 4:06 p.m.

Details

Message ID 20180215160625.75406-5-christophe@dinechin.org
State New
Headers show
Series "Updates to the style guide" ( rev: 3 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 15, 2018, 4:06 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

Changes since v3:
- Integrate comments about performance impact and solution
- Integrate comments about impact of a failed assertion
- Add prohibition against side effects

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 docs/spice_style.txt | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index 5dc41cb1..3bc70570 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -113,10 +113,40 @@  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
+----------
+
+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.
+
+Use assertions liberally, but pay attention to performance impact. Assertions checks have a significant performance impact should be placed under the `ENABLE_EXTRA_CHECKS` conditional.
+
+[source,c]
+----
+void function(void)
+{
+    while (condition()) { // Hot loop
+        spice_assert(ENABLE_EXTRA_CHECKS && expensive_check());
+    }
+    ...
+}
+----
+
+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 disabled 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.
+
+Be mindful of the impact of a failing assertion. For example, assertions in the server should not abort it, as this would kill the QEMU process and the virtual machine.
+
+Assertions should not:
+
+- Be used as a replacement for proper error management or to check unsafe data such as user input
+- Have side effects, since an assertion check may be disabled
+- Call functions, since this may lead to side effects and performance impact
 
-Use it freely. ASSERT helps testing function arguments and function results validity.  ASSERT helps detecting bugs and improve code readability and stability.
 
 sizeof
 ------

Comments

On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Changes since v3:
> - Integrate comments about performance impact and solution
> - Integrate comments about impact of a failed assertion
> - Add prohibition against side effects
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  docs/spice_style.txt | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 5dc41cb1..3bc70570 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -113,10 +113,40 @@ 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, but pay attention to performance impact. Assertions checks have a significant performance impact should be placed under the `ENABLE_EXTRA_CHECKS` conditional.

[snip]

> +
> +Be mindful of the impact of a failing assertion. For example, assertions in the server should not abort it, as this would kill the QEMU process and the virtual machine.

For me, "assertion" implies abortion of the process, so the first
sentence ("use assertions liberally") and this one are confusing.
In particular, g_assert() which you mention in the snipped part does
abort.

Christophe
> On 16 Feb 2018, at 10:12, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> Changes since v3:
>> - Integrate comments about performance impact and solution
>> - Integrate comments about impact of a failed assertion
>> - Add prohibition against side effects
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> docs/spice_style.txt | 36 +++++++++++++++++++++++++++++++++---
>> 1 file changed, 33 insertions(+), 3 deletions(-)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index 5dc41cb1..3bc70570 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -113,10 +113,40 @@ 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, but pay attention to performance impact. Assertions checks have a significant performance impact should be placed under the `ENABLE_EXTRA_CHECKS` conditional.
> 
> [snip]
> 
>> +
>> +Be mindful of the impact of a failing assertion. For example, assertions in the server should not abort it, as this would kill the QEMU process and the virtual machine.
> 
> For me, "assertion" implies abortion of the process,

Which is why I point out the impact.

> so the first
> sentence ("use assertions liberally") and this one are confusing.
> In particular, g_assert() which you mention in the snipped part does
> abort.

So? I don’t understand the problem. How would you rephrase?

> 
> Christophe
On Fri, Feb 16, 2018 at 10:40:58AM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 16 Feb 2018, at 10:12, Christophe Fergeau <cfergeau@redhat.com> wrote:
> > 
> > On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin <dinechin@redhat.com>
> >> 
> >> Changes since v3:
> >> - Integrate comments about performance impact and solution
> >> - Integrate comments about impact of a failed assertion
> >> - Add prohibition against side effects
> >> 
> >> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> >> ---
> >> docs/spice_style.txt | 36 +++++++++++++++++++++++++++++++++---
> >> 1 file changed, 33 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> >> index 5dc41cb1..3bc70570 100644
> >> --- a/docs/spice_style.txt
> >> +++ b/docs/spice_style.txt
> >> @@ -113,10 +113,40 @@ 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, but pay attention to performance impact. Assertions checks have a significant performance impact should be placed under the `ENABLE_EXTRA_CHECKS` conditional.
> > 
> > [snip]
> > 
> >> +
> >> +Be mindful of the impact of a failing assertion. For example, assertions in the server should not abort it, as this would kill the QEMU process and the virtual machine.
> > 
> > For me, "assertion" implies abortion of the process,
> 
> Which is why I point out the impact.
> 
> > so the first
> > sentence ("use assertions liberally") and this one are confusing.
> > In particular, g_assert() which you mention in the snipped part does
> > abort.
> 
> So? I don’t understand the problem. How would you rephrase?

I read the beginning as saying "use g_assert() whenever you want", and
the end saying "be very careful with g_assert() because it kills the
program".
In general, I would rework this section to be about assertions and
preconditions.
preconditions in the form of g_return_if_*, g_warn_if_*, ... can be used
liberally, should be used on most public entry points (maybe even on
non-static functions), but only to catch a situation where we have a
programming error, ie something which the programmer thinks should not
happen the way the code is written.
assertions in the form of g_assert (we want to deprecate spice helpers
which have glib equivalent) should be used very rarely, ideally never.

Christophe
On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Changes since v3:
> - Integrate comments about performance impact and solution
> - Integrate comments about impact of a failed assertion
> - Add prohibition against side effects
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  docs/spice_style.txt | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index 5dc41cb1..3bc70570 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -113,10 +113,40 @@ 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
> +----------
> +
> +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.
> +
> +Assertions checks have a significant performance impact should be placed under the `ENABLE_EXTRA_CHECKS` conditional.

"which have" ?

> +
> +[source,c]
> +----
> +void function(void)
> +{
> +    while (condition()) { // Hot loop
> +        spice_assert(ENABLE_EXTRA_CHECKS && expensive_check());
> +    }
> +    ...
> +}
> +----
> +
> +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 disabled 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.

I would expect program abortion to be mentioned here in addition to the
printing of the message.

> +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'd remove this and just say to use g_*

> +
> +Be mindful of the impact of a failing assertion. For example, assertions in the server should not abort it, as this would kill the QEMU process and the virtual machine.
> +
> +Assertions should not:
> +
> +- Be used as a replacement for proper error management or to check unsafe data such as user input
> +- Have side effects, since an assertion check may be disabled

*if* we really plan to disable assertions in some cases, I think we'd
need to be more explicit about when it's going to happen. If we don't
consider it a valid usecase to disable assertions, then we can drop
this.