| Message ID | 1257960543-26373-2-git-send-email-aliguori@us.ibm.com |
|---|---|
| State | New |
| Headers | show |
Am 11.11.2009 18:28, schrieb Anthony Liguori: > This makes lists no longer invariant. It's a very useful bit of functionality > though. > > To deal with the fact that lists are no longer invariant, introduce a deep > copy mechanism for lists. > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > --- > qlist.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > qlist.h | 4 ++++ > 2 files changed, 60 insertions(+), 0 deletions(-) So far all functions in qlist.c have a header comment. Any reason to change this? Kevin
Kevin Wolf wrote: > Am 11.11.2009 18:28, schrieb Anthony Liguori: > >> This makes lists no longer invariant. It's a very useful bit of functionality >> though. >> >> To deal with the fact that lists are no longer invariant, introduce a deep >> copy mechanism for lists. >> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> --- >> qlist.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> qlist.h | 4 ++++ >> 2 files changed, 60 insertions(+), 0 deletions(-) >> > > So far all functions in qlist.c have a header comment. Any reason to > change this? > But nothing else in qemu does. I don't find this commenting style particularly helpful since they really don't tell you anything you can't infer from the function name. I'm not opposed to people adding these types of comments but I don't think it's a good idea to attempt to enforce it in just this one place (not that I think it should be enforced everywhere). Regards, Anthony Liguori > Kevin > > >
Am 12.11.2009 17:46, schrieb Anthony Liguori: > Kevin Wolf wrote: >> Am 11.11.2009 18:28, schrieb Anthony Liguori: >> >>> This makes lists no longer invariant. It's a very useful bit of functionality >>> though. >>> >>> To deal with the fact that lists are no longer invariant, introduce a deep >>> copy mechanism for lists. >>> >>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >>> --- >>> qlist.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> qlist.h | 4 ++++ >>> 2 files changed, 60 insertions(+), 0 deletions(-) >>> >> >> So far all functions in qlist.c have a header comment. Any reason to >> change this? >> > > But nothing else in qemu does. Unfortunately. There are places where such comments could be a good specification on what an interface is actually meant to work like (particularly in error cases). Currently you often can't tell if the implementation or the caller of a function is buggy. Not sure if they are really useful for the simple qlist.c functions (but even there the function name does not tell me what it's doing with NULL parameters), but it might be helpful to have a general discussion about it. I think in general qemu is poorly commented. Kevin
Kevin Wolf wrote: > Unfortunately. There are places where such comments could be a good > specification on what an interface is actually meant to work like > (particularly in error cases). Currently you often can't tell if the > implementation or the caller of a function is buggy. > > Not sure if they are really useful for the simple qlist.c functions (but > even there the function name does not tell me what it's doing with NULL > parameters), but it might be helpful to have a general discussion about > it. I think in general qemu is poorly commented. > I agree, but I don't think the solution is forcing boiler plate commenting styles. I think what we could improve on is asking people to comment bits of code during review. > Kevin >
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Kevin Wolf wrote:
> I think in general qemu is poorly commented.
I think thats a bit of an understatement. in particular I'm finding the
options code to be a nightmare. Hopefully I'll be fixing some of that in
my upcomming submissions.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/
iQIcBAEBAgAGBQJK/EMMAAoJEFIjE1w7L6YH2BgP/17YUHz92sD001Q8VxETeTJS
RhKONA6tX4vmnKfUTkM9VrqV3Qv7lyhhGN2IdJulUpKEKRmoykMuPKopkaYKuCJ4
9WzxQOimAHQbZ9NrPYulafygCx4opiuzFenuCoiabzdN6qhT7zdBmhqIMH188f/G
W8Bc4c0eiPD0wWwaomr4VMh+BaJ2lZDElPNNz7FxoUAdoedoNs0Iyj8VxlBGzNSz
6IoIqCBLgTzMbSKLIeKZ3eKGK3AZDIPpWbA7qlsB2K2zUQW9nlNVgs8BbOihSTng
NN2+9e0/QQvYGa/xYaGc/gmXf9WZwhMncvhPyuwB6alsMNzNF6X3jfvqC6ZDkzq5
NVNI6ItN1cVvbJqTPLGQ6EHspO9Du8GssbQnPiM3wCTmsB9aAV65LOWyBxigAdz1
7gQvXxt2+pDduqahOdw3vc2DllhtkASSH1JIPM90Hc/H6qHCBdkQO3okqZr/rOm5
OiQwONQ84k5Pp6GRcuWPbl+Dnrs47P4N7wQ876CooqGxhn8z4JIk6jKpIPqsDWVn
rxWVF41Hbxr/92BVrxBqmAJlIaqUMnUhTfpiYPVdaKMcrRIDWXpDXQOlNrztdk3a
INcv4NhJ604ab5UpLO3WbLIJK4CTnRXldly++3DHM60im6fO1kCsq5sKe56V7lNi
WossoFOnMg5Xb9RBxWxo
=iEnN
-----END PGP SIGNATURE-----
On Thu, 12 Nov 2009 11:13:45 -0600 Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote: > Kevin Wolf wrote: > > Unfortunately. There are places where such comments could be a good > > specification on what an interface is actually meant to work like > > (particularly in error cases). Currently you often can't tell if the > > implementation or the caller of a function is buggy. > > > > Not sure if they are really useful for the simple qlist.c functions (but > > even there the function name does not tell me what it's doing with NULL > > parameters), but it might be helpful to have a general discussion about > > it. I think in general qemu is poorly commented. > > > > I agree, but I don't think the solution is forcing boiler plate > commenting styles. I think what we could improve on is asking people to > comment bits of code during review. I've started adding comments like that because this is an API which is probably going to be part of a library, as such it has to be properly documented and very likely to be generated automatically by tools like doxygen. If we do this we should be consistent and document everything which is public, even simple cases.
Am 12.11.2009 18:13, schrieb Anthony Liguori: > Kevin Wolf wrote: >> Unfortunately. There are places where such comments could be a good >> specification on what an interface is actually meant to work like >> (particularly in error cases). Currently you often can't tell if the >> implementation or the caller of a function is buggy. >> >> Not sure if they are really useful for the simple qlist.c functions (but >> even there the function name does not tell me what it's doing with NULL >> parameters), but it might be helpful to have a general discussion about >> it. I think in general qemu is poorly commented. >> > > I agree, but I don't think the solution is forcing boiler plate > commenting styles. I think what we could improve on is asking people to > comment bits of code during review. Be sure that I'll be asking for comments in qcow2 patches. I did in the past and I'll continue to do so. ;-) I agree that boiler plates are not going to help per se. But I think what actually does play a role in commenting is what surrounding code looks like and it's also habits. And it's probably not wrong to say that we're currently in a habit of not documenting anything. If boiler plates in some places are the price to get into a habit of documenting things, I think considering to accept that price wouldn't be completely unreasonable. Another thing is that we could ask more often to move explanations from mails into the code. The explanation often exists and sometimes we're lucky enough that it ends up at least in the commit log, but there are cases where it's in PATCH 0/n or buried in the corresponding mail thread. Kevin
diff --git a/qlist.c b/qlist.c index ba2c66c..5fccb7d 100644 --- a/qlist.c +++ b/qlist.c @@ -37,6 +37,23 @@ QList *qlist_new(void) return qlist; } +static void qlist_copy_elem(QObject *obj, void *opaque) +{ + QList *dst = opaque; + + qobject_incref(obj); + qlist_append_obj(dst, obj); +} + +QList *qlist_copy(QList *src) +{ + QList *dst = qlist_new(); + + qlist_iter(src, qlist_copy_elem, dst); + + return dst; +} + /** * qlist_append_obj(): Append an QObject into QList * @@ -67,6 +84,45 @@ void qlist_iter(const QList *qlist, iter(entry->value, opaque); } +QObject *qlist_pop(QList *qlist) +{ + QListEntry *entry; + QObject *ret; + + if (qlist == NULL || QTAILQ_EMPTY(&qlist->head)) { + return NULL; + } + + entry = QTAILQ_FIRST(&qlist->head); + QTAILQ_REMOVE(&qlist->head, entry, next); + + ret = entry->value; + qemu_free(entry); + + return ret; +} + +QObject *qlist_peek(QList *qlist) +{ + QListEntry *entry; + QObject *ret; + + if (qlist == NULL || QTAILQ_EMPTY(&qlist->head)) { + return NULL; + } + + entry = QTAILQ_FIRST(&qlist->head); + + ret = entry->value; + + return ret; +} + +int qlist_empty(const QList *qlist) +{ + return QTAILQ_EMPTY(&qlist->head); +} + /** * qobject_to_qlist(): Convert a QObject into a QList */ diff --git a/qlist.h b/qlist.h index 3eb1eb8..afdc446 100644 --- a/qlist.h +++ b/qlist.h @@ -30,9 +30,13 @@ typedef struct QList { qlist_append_obj(qlist, QOBJECT(obj)) QList *qlist_new(void); +QList *qlist_copy(QList *src); void qlist_append_obj(QList *qlist, QObject *obj); void qlist_iter(const QList *qlist, void (*iter)(QObject *obj, void *opaque), void *opaque); +QObject *qlist_pop(QList *qlist); +QObject *qlist_peek(QList *qlist); +int qlist_empty(const QList *qlist); QList *qobject_to_qlist(const QObject *obj); #endif /* QLIST_H */
This makes lists no longer invariant. It's a very useful bit of functionality though. To deal with the fact that lists are no longer invariant, introduce a deep copy mechanism for lists. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- qlist.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qlist.h | 4 ++++ 2 files changed, 60 insertions(+), 0 deletions(-)