diff options
author | David Laban <david.laban@collabora.co.uk> | 2011-05-19 17:15:15 -0400 |
---|---|---|
committer | David Laban <david.laban@collabora.co.uk> | 2011-05-19 17:15:15 -0400 |
commit | 30f8e28185261a9ba910a4b6e3cf3d2b65ec9e61 (patch) | |
tree | 820edd7c8f42262ed49eedd5526b15bde653be13 | |
parent | 023fd3ba6e689cd720b8bbd4af7dbd3757162407 (diff) | |
download | telepathy-logger-30f8e28185261a9ba910a4b6e3cf3d2b65ec9e61.tar.gz |
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.
-rw-r--r-- | telepathy-logger/text-event-internal.h | 2 | ||||
-rw-r--r-- | telepathy-logger/text-event.c | 22 | ||||
-rw-r--r-- | telepathy-logger/text-event.h | 2 |
3 files changed, 12 insertions, 14 deletions
diff --git a/telepathy-logger/text-event-internal.h b/telepathy-logger/text-event-internal.h index 8883d57..4a68a88 100644 --- a/telepathy-logger/text-event-internal.h +++ b/telepathy-logger/text-event-internal.h @@ -55,5 +55,7 @@ gint _tpl_text_event_get_pending_msg_id (TplTextEvent *self); gboolean _tpl_text_event_is_pending (TplTextEvent *self); +void _tpl_text_event_add_supersedes (TplTextEvent *self, + TplTextEvent *old_event); G_END_DECLS #endif // __TPL_TEXT_EVENT_INTERNAL_H__ 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; diff --git a/telepathy-logger/text-event.h b/telepathy-logger/text-event.h index 1648d8c..f5ee64a 100644 --- a/telepathy-logger/text-event.h +++ b/telepathy-logger/text-event.h @@ -45,8 +45,6 @@ const gchar *tpl_text_event_get_message (TplTextEvent *self); const gchar *tpl_text_event_get_message_token (TplTextEvent *self); const gchar *tpl_text_event_get_supersedes_token (TplTextEvent *self); -void tpl_text_event_add_supersedes (TplTextEvent *self, - TplTextEvent *old_event); GList *tpl_text_event_dup_supersedes (TplTextEvent *self); G_END_DECLS |