On Fri, Feb 16, 2018 at 10:40:58AM +0100, Christophe de Dinechin wrote:
>
>
> > On 16 Feb 2018, at 10:12, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
> >
> > On Thu, Feb 15, 2018 at 05:06:17PM +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin <dinechin@xxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxx>
> >> ---
> >> 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel