[Bug 24618] [0.11] add TpStringPool, a refcounted pool of strings

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=24618

--- Comment #8 from Cosimo Alfarano <cosimo.alfarano at collabora.co.uk> 2010-04-21 07:43:06 PDT ---
Comment related to comment#6
I ignored the comments related to the out-of-date branch you used when they
where not applicable (usually already fixed).

Shortly: fixed the rest of comments. not ready for next review cycle.

>> + * Copyright (C) 2005, 2007 Collabora Ltd.
>> + * Copyright (C) 2005, 2007 Nokia Corp.

> Please extend the Collabora copyright range to 2005-2010, leaving the Nokia one
> as-is.

OK. Updated both handle-repo-dynamic.[ch] with 2005-2010 and changed
string-pool.h from "2005, 2007, 2010" to "2005-2010".

>> +  self->string_pool = tp_string_pool_new (NULL, NULL);
>> +
>> +  if (self->string_pool == NULL)
>> +    g_error ("Unable to create the string pool");
> Why would this ever fail? If the answer is OOM, you don't need to check for
> that - GLib calls g_error on OOM anyway.

Removed.

> +  if (self->string_pool != NULL)
> +    {
> +      g_object_unref (self->string_pool);
> +      self->string_pool = NULL;
> +    }

> This is releasing a reference, so it should be in dispose().

Right.

> Why don't you just call the ensure() function to start with?

fixed. I tried to keep my code as much similar as I could to the old one, but
this was silly.


>> +guint tp_string_pool_lookup_exact (TpStringPool *self,
>> +    const char *id);

> I think it'd be worth having the TpStringPool's API guarantee that its numbers
> are guint32 (in practice, that's what TpDynamicHandleRepo has always done, and
> numbers above G_MAXUINT32 can't be Telepathy handles anyway).

>I'm not sure about the API naming: perhaps something like this, replacing
>NUMBER with whatever handle-like concept we come up with? (GQuark is another
>good source of naming conventions for this.)

wrt the #telepathy chat we had, I'm going to use the following instead:

>/* does not return a ref */
>guint32 tp_string_pool_lookup_NUMBER (TpStringPool *self, const gchar *s);

tp_string_pool_lookup_by_string()
from tp_string_pool_lookup()

/* returns a ref */
> guint32 tp_string_pool_ensure_NUMBER (TpStringPool *self, const gchar *s)
> G_GNUC_WARN_UNUSED_RESULT;

the same.
from tp_string_pool_ensure()

> const gchar *tp_string_pool_get_string (TpStringPool *self, guint32 NUMBER);

tp_string_pool_lookup_by_NUMBER()
from tp_string_pool_lookup_by_handle()

> gboolean tp_string_pool_string_exists (TpStringPool *self, const gchar *s);

not existing, added.

> gboolean tp_string_pool_NUMBER_exists (TpStringPool *self, guint32 NUMBER);

the same
from tp_string_pool_is_valid()

> /* ref should usually return the thing that was reffed */
> guint32 tp_string_pool_ref_NUMBER (TpStringPool *self, guint32 NUMBER);

the same
from tp_string_pool_ref_handle()

> void tp_string_pool_unref_NUMBER (TpStringPool *self, guint32 NUMBER);

the same
from tp_string_pool_unref_handle()

> I'm not sure that we need tp_string_pool_handles_are_valid - the only reason
> for taking a GArray there is to be nice to D-Bus APIs that take an array of
> handles. I'd be inclined to do the iteration in the dynamic handle repo
> instead.

removed in TpStringPool, the dyn.repo is already iterating. It's using
dynamic_handle_is_valid() instead of tp_string_pool_NUMBER_exists(), I usually
think it's more readable. Is it OK?


> +static inline TpStringPool *
> +tp_string_pool_new (TpStringPoolNormalizeFunc normalize_func,
> +    gpointer default_normalize_context)

> I think making this inline is overkill. It can just be a normal function; it's
> not as if it will be called very often.

OK, it was defined that way in the handle repo headers. fixed.

> +tp_string_pool_ensure_handle (TpStringPool *self,
...
> +  g_return_val_if_fail (!tp_str_empty (id), 0);

> A string pool should be able to store "". It's OK if it can't store NULL,
> although that fact should probably be documented. (lookup, lookup_exact have
> the same problem).

Empty string is legal, updated sanity checks and doc.

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.