Re: [Qemu-devel] [PATCH 04/17] qapi: merge QInt and QFloat in QNum

[Luiz Capitulino <lcapitulino@redhat.com>]

On Fri, 12 May 2017 08:30:36 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Question for Luiz...
> 
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
> 
> [...]
> > diff --git a/tests/check-qnum.c b/tests/check-qnum.c
> > new file mode 100644
> > index 0000000000..d08d35e85a
> > --- /dev/null
> > +++ b/tests/check-qnum.c
> > @@ -0,0 +1,131 @@
> > +/*
> > + * QNum unit-tests.
> > + *
> > + * Copyright (C) 2009 Red Hat Inc.
> > + *
> > + * Authors:
> > + *  Luiz Capitulino <lcapitulino@redhat.com>
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
> > + * See the COPYING.LIB file in the top-level directory.
> > + */
> > +#include "qemu/osdep.h"
> > +
> > +#include "qapi/qmp/qnum.h"
> > +#include "qapi/error.h"
> > +#include "qemu-common.h"
> > +
> > +/*
> > + * Public Interface test-cases
> > + *
> > + * (with some violations to access 'private' data)
> > + */
> > +
> > +static void qnum_from_int_test(void)
> > +{
> > +    QNum *qi;
> > +    const int value = -42;
> > +
> > +    qi = qnum_from_int(value);
> > +    g_assert(qi != NULL);
> > +    g_assert_cmpint(qi->u.i64, ==, value);
> > +    g_assert_cmpint(qi->base.refcnt, ==, 1);
> > +    g_assert_cmpint(qobject_type(QOBJECT(qi)), ==, QTYPE_QNUM);
> > +
> > +    // destroy doesn't exit yet
> > +    g_free(qi);
> > +}  
> 
> The comment is enigmatic. 

It was meant for future generations to figure it out :)

> It was first written in commit 33837ba
> "Introduce QInt unit-tests", and got copied around since.  In
> check-qlist.c, it's spelled "exist yet".

Yes, "exit" is a typo it should be "exist".

> What is "destroy", why doesn't it exit / exist now, but will exit /
> exist later?  It can't be qnum_destroy_obj(), because that certainly
> exists already, exits already in the sense of returning, and shouldn't
> ever exit in the sense of terminating the program.
> 
> The comment applies to a g_free().  Why do we free directly instead
> decrementing the reference count?  Perhaps the comment tries to explain
> that (if it does, it fails).

In my personal style of writing unit-tests, I never use a method
in a test before testing it. So, as QDECREF() wasn't tested yet,
I wasn't allowed to use it.

While I keep this principle when writing unit-tests today, this
particular case is very extreme and not useful at all. Today I'd just
go ahead and use QDECREF(). The qint_destroy_test() in the original
commit is also very bogus, it's not really doing an useful test.