[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/17] qapi: merge QInt and QFloat in QNum
From: |
Luiz Capitulino |
Subject: |
Re: [Qemu-devel] [PATCH 04/17] qapi: merge QInt and QFloat in QNum |
Date: |
Fri, 12 May 2017 09:00:32 -0400 |
On Fri, 12 May 2017 08:30:36 +0200
Markus Armbruster <address@hidden> wrote:
> Question for Luiz...
>
> Marc-André Lureau <address@hidden> 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 <address@hidden>
> > + *
> > + * 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.
- Re: [Qemu-devel] [PATCH 01/17] qdev: remove PropertyInfo.qtype field, (continued)
[Qemu-devel] [PATCH 05/17] qapi: remove promote_int, Marc-André Lureau, 2017/05/09
[Qemu-devel] [PATCH 06/17] qnum: add uint type, Marc-André Lureau, 2017/05/09
[Qemu-devel] [PATCH 07/17] json: learn to parse uint64 numbers, Marc-André Lureau, 2017/05/09