Bug 27400

Short version: please apply the Principle of Least Astonishment whenever you design APIs :-)

In the first patch it seems you're changing the semantics of tpl_channel_text_clean_up_stale_tokens from "free the list and the contained strings" to "free the list, don't free the contained strings". Are you sure this isn't a memory leak?

In any case, I think this signature is extremely surprising - functions shouldn't take ownership of (i.e. destroy) one of their arguments unless the name of the function makes this extremely obvious. I'd prefer it like this:

/* make it iterate non-destructively */
void tpl_channel_text_clean_up_stale_tokens (..., const GList *);

/* at each use: */
tpl_channel_text_clean_up_stale_tokens (..., list);
g_list_foreach (list, (GFunc) g_free, NULL);    /* if needed */
g_list_free (list);

In the second patch I think the argument change is a necessary evil with the API as currently designed, but it surprises me that a function called tpl_channel_text_msg_token_exist_in_cache() has side-effects. (Also, it should have been called ..._exists_... anyway.)

Please rename this function to tpl_channel_text_msg_token_remove_from_cache() to make the side-effect obvious - there is considerable precedent for a "remove" method returning a gboolean to say whether it actually removed anything (e.g. g_hash_table_remove()).

> + * frees the list and its items (char *).

We use gchar * for strings allocated by the g_malloc/g_free family, and char * for strings allocated by something else (e.g. malloc/free, xmlMalloc/xmlFree) - g_malloc is not guaranteed to be compatible with malloc on all platforms.

>  * used by:

There's no need for this; please assume that future developers are capable of operating grep correctly :-)

> +  if (g_list_length (l) > 1)
> +    CRITICAL ("found more than one log-id %s, it shouldn't be possible",
> +        tpl_message_token);

No. Please think about GList's underlying structure, and why this assertion will fail.

Suppose we have this valid list (pretend for a moment that l->data was a simple string and not some sort of struct):

["one", "two", "three"]

Because GList is a doubly-linked list, this is actually represented as:

l1 = { .data = "one", .prev = NULL , .next = l2 }
l2 = { .data = "two", .prev = l1 , .next = l3 }
l3 = { .data = "three", .prev = l2 , .next = NULL }

Now search for "one" or "two"; you'll get l1 or l2. The g_list_length() will be 3 or 2, respectively, and the assertion will fail.

(Even in situations where this would work, "g_list_length (foo) > 1" is just an inefficient O(n) way to spell "foo != NULL && foo->next != NULL", which is O(1).)

The rest looks OK.

Summary:

- tpl_channel_text_msg_token_exist_in_cache() removed
- side-effect removed from tpl_channel_text_clean_up_stale_tokens()

I rebased the branch to remove the old patches.

> In the first patch it seems you're changing the semantics of
> tpl_channel_text_clean_up_stale_tokens from "free the list and the contained
> strings" to "free the list, don't free the contained strings". Are you sure
> this isn't a memory leak?

Yes, introduced but not meant.
I removed the side-effect, removing this code as well.

> In the second patch I think the argument change is a necessary evil with the
> API as currently designed, but it surprises me that a function called

The intent was grouping repeated code for readability, the result is much more readable when it isn't grouped, so I removed the method.

I'd like to express better why I'm removing the item from the list, which is the key point.
I don't know if it's clear enough: lines 773 and 783, repeated at 889 and 899.


> tpl_channel_text_msg_token_exist_in_cache() has side-effects. (Also, it should
> have been called ..._exists_... anyway.)
> Please rename this function to tpl_channel_text_msg_token_remove_from_cache()

I removed the function.

> + * frees the list and its items (char *).
> We use gchar * for strings allocated by the g_malloc/g_free family, and char *

Comment actually removed because of the removal of the side-effect.

>> +  if (g_list_length (l) > 1)
>> +    CRITICAL ("found more than one log-id %s, it shouldn't be possible",
>> +        tpl_message_token);
> No. Please think about GList's underlying structure, and why this assertion
> will fail.

Removed with tpl_channel_text_msg_token_exist_in_cache()

This looks better, thanks. One remaining thing:

> http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=commitdiff;h=7ed867ad8231bc

In this patch, just after each of two calls to g_list_find_custom, you have a block of this form:

if (foo)
  bar ();
else
  {
    badger ();
    mushroom ();
  }

Please add {} around the call to "bar" in structures of this type: each if/else pair (and each set of if/else if/.../else) should consistently either have, or not have, {} on each block (i.e. if one block has more than one statement, then they all need {}).

Ship it.

Merged to master.