[Spice-devel] [PATCH v4 04/12] Rephrase assertion section

[Christophe de Dinechin]

From: Christophe de Dinechin <dinechin at 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 at 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.
+
+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
 ------
-- 
2.13.5 (Apple Git-94)