[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 #6 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-04-20 04:18:19 PDT ---
Makefile.am needs to add string-pool.h to the HEADERS.

telepathy-glib.h should perhaps include string-pool.h.

> + * SECTION:string-pool
> + * @title: TpStringPool
> + * @short_description: arbitrary string pool, with dynamic handle allocation
> + * and recycling.
> + *
> + * The #TpStringPool:normalize-function property may be set to
> + * perform validation and normalization on handle ID strings.

I think a better description would be:

 * @short_description: a pool of reference-counted strings identified by number
 * 
 * This string pool implementation is a generalization of #GQuark.
 * Whereas the #GQuark mechanism leaves strings allocated permanently,
 * each #TpStringPool maintains a reference count for each string it contains.

> + * 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.

> -static void dynamic_repo_iface_init (gpointer g_iface,
> +static inline void dynamic_repo_iface_init (gpointer g_iface,

There's no point in inlining this: G_IMPLEMENT_INTERFACE takes the address of
the iface_init function, so it can't be inlined.

> +  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.

> +  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().

> @@ -810,26 +690,18 @@ dynamic_ensure_handle (TpHandleRepoIface *irepo,
...
> +  handle = tp_string_pool_lookup_exact (self->string_pool, id);
> +  if (handle != 0)
...
> +  handle = tp_string_pool_ensure_handle (self->string_pool, id, context, error)

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

> + * The #TpStringPool:normalize-function property may be set to
> + * perform validation and normalization on handle ID strings.

I still don't think TpStringPool should have normalization. You're not using it
in TpDynamicHandleRepo, and neither Mission Control nor Gabble is going to use
it either.

If in doubt, remove it for now, and we can reinstate it later if necessary.

I haven't reviewed normalization functionality further.

> +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.)

/* does not return a ref */
guint32 tp_string_pool_lookup_NUMBER (TpStringPool *self, const gchar *s);
/* returns a ref */
guint32 tp_string_pool_ensure_NUMBER (TpStringPool *self, const gchar *s)
G_GNUC_WARN_UNUSED_RESULT;
const gchar *tp_string_pool_get_string (TpStringPool *self, guint32 NUMBER);
gboolean tp_string_pool_string_exists (TpStringPool *self, const gchar *s);
gboolean tp_string_pool_NUMBER_exists (TpStringPool *self, guint32 NUMBER);
/* ref should usually return the thing that was reffed */
guint32 tp_string_pool_ref_NUMBER (TpStringPool *self, guint32 NUMBER);
void tp_string_pool_unref_NUMBER (TpStringPool *self, guint32 NUMBER);

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.

What's the difference between tp_string_pool_inspect_handle and
tp_string_pool_lookup_handle? Are they, in fact, the same?

> +#include <config.h>
> +
> +#include <telepathy-glib/errors.h>
> +#include <telepathy-glib/heap.h>
> +#include <telepathy-glib/util.h>
> +#include <telepathy-glib/string-pool.h>

Please put the file's own header (string-pool.h) immediately after config.h, as
an easy way to verify that it's self-contained.

> +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.

> +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).

A lot of the functions don't seem to be documented. Please do so (it should be
much easier once you've got rid of normalization and redundant API).

> +const gchar *tp_string_pool_lookup_handle (TpStringPool *self,
> +    guint handle)
> +{
> +  g_return_val_if_fail (handle != 0, NULL);
> +  g_return_val_if_fail (TP_IS_STRING_POOL (self), NULL);
> +
> +  return g_hash_table_lookup (self->priv->handle_to_priv,
> +      GUINT_TO_POINTER (handle));

This seems to return a HandlePriv * (an opaque struct that library users can't
interpret), rather than the string you'd expect it to return, which makes the
whole function rather useless. Why does this function exist?

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