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

[Christophe Fergeau]

On Fri, Feb 16, 2018 at 10:40:58AM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 16 Feb 2018, at 10:12, Christophe Fergeau <cfergeau at redhat.com> wrote:
> > 
> > On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> >> 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
> >> +----------
> >> +
> >> +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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180216/27c7321d/attachment.sig>