From 30f8e28185261a9ba910a4b6e3cf3d2b65ec9e61 Mon Sep 17 00:00:00 2001 From: David Laban Date: Thu, 19 May 2011 17:15:15 -0400 Subject: fixup! tpl_text_event_{add,dup}_supersedes Review comments addressed: > g_list_free_full() is glib 2.28 while the configure.ac says we need 0.2.25.11. > We usually to a g_list_foreach(list, unref), g_list_free() so we don't have to > bump to very recent glib version. > > Also,I strongly prefer if you clear the queue structure, and not keep random > pointers around then using this boolean. Also, as NULL is a correct empty list, > you don't have to do any NULL check. > > > Patch: "tpl_text_event_{add,dup}_supersedes": > > add_supersedes() writes into a TplTextEvent, this is not allowed in the public > interface, move this function to text-event-private.h and prepend a _. > > > + for (l = old_event->priv->supersedes.head; l != NULL; l = l->next) > > + g_queue_push_tail (&self->priv->supersedes, g_object_ref (l->data)); > > Use g_list_next() instead of "l = l->next" for readability, don't worry it's a > macro, not a function call. > > > + for (l = self->priv->supersedes.tail; l != NULL; l = l->prev) > > + supersedes = g_list_prepend (supersedes, g_object_ref (l->data)); > > Use g_list_previous(). > Also, I realised that my annotation (transfer full) was incorrect: Since I ref the object that is passed in, I think (transfer none) is the correct annotation (since I create a new ref rather than stealing it off the caller). Correct me if I'm wrong again. --- telepathy-logger/text-event.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) (limited to 'telepathy-logger/text-event.c') diff --git a/telepathy-logger/text-event.c b/telepathy-logger/text-event.c index a8ba21b..f4f9c00 100644 --- a/telepathy-logger/text-event.c +++ b/telepathy-logger/text-event.c @@ -58,7 +58,6 @@ struct _TplTextEventPriv /* A list of TplTextEvent that we supersede. * This is only populated when reading logs (not storing them). */ GQueue supersedes; - gboolean dispose_has_run; }; enum @@ -84,11 +83,9 @@ tpl_text_event_dispose (GObject *obj) { TplTextEventPriv *priv = TPL_TEXT_EVENT (obj)->priv; - if (priv->dispose_has_run) - return; - priv->dispose_has_run = TRUE; - - g_list_free_full (priv->supersedes.head, g_object_unref); + g_list_foreach (priv->supersedes.head, (GFunc) g_object_unref, NULL); + g_list_free (priv->supersedes.head); + g_queue_init (&priv->supersedes); } @@ -322,16 +319,16 @@ tpl_text_event_get_supersedes_token (TplTextEvent *self) /** - * tpl_text_event_add_supersedes + * _tpl_text_event_add_supersedes * @self: a #TplTextEvent - * @old_event: (transfer full): an #TplTextEvent which this one supersedes + * @old_event: (transfer none): an #TplTextEvent which this one supersedes * * If there are other known entries in the message edit/succession chain, * they should be added to old_event before linking these two events, * as they will be copied onto this event for convenience. */ void -tpl_text_event_add_supersedes (TplTextEvent *self, +_tpl_text_event_add_supersedes (TplTextEvent *self, TplTextEvent *old_event) { GList *l; @@ -339,7 +336,7 @@ tpl_text_event_add_supersedes (TplTextEvent *self, g_object_ref (old_event); g_queue_push_tail (&self->priv->supersedes, old_event); - for (l = old_event->priv->supersedes.head; l != NULL; l = l->next) + for (l = old_event->priv->supersedes.head; l != NULL; l = g_list_next (l)) g_queue_push_tail (&self->priv->supersedes, g_object_ref (l->data)); if (self->priv->supersedes_token == NULL) @@ -352,7 +349,8 @@ tpl_text_event_add_supersedes (TplTextEvent *self, * @self: a #TplTextEvent * * Returns: (transfer full): A #GList of #TplTextEvent that this event - * supersedes. Should be freed using g_list_free_full (l, g_object_unref). + * supersedes. Should be freed using + * g_list_foreach (l, g_object_unref, NULL); g_list_free (l). */ GList * tpl_text_event_dup_supersedes (TplTextEvent *self) @@ -361,7 +359,7 @@ tpl_text_event_dup_supersedes (TplTextEvent *self) GList *l; /* Iterate backwards to copy quickly (thanks GList) */ - for (l = self->priv->supersedes.tail; l != NULL; l = l->prev) + for (l = self->priv->supersedes.tail; l != NULL; l = g_list_previous (l)) supersedes = g_list_prepend (supersedes, g_object_ref (l->data)); return supersedes; -- cgit v1.2.1