Re: [PATCH 3/6] core: made plugin registry handle itself key id assignments

[Iago Toral Quiroga <itoral igalia com>]

El mié, 11-05-2011 a las 20:31 +0200, Guillaume Emont escribió:
> Instead of relying on GQuarks, a similar mechanism is implemented in the plugin
> registry. This gives more flexibility and allows  to #define the values of core
> keys, which automatically give their symbols in GObject-introspection bindings.

Not sure I follow this properly, can you elaborate on the benefits we
obtain with this solution for the introspection based bindings?

> ---
>  src/grl-plugin-registry.c |  229 +++++++++++++++++++++++++++++++--------------
>  src/grl-plugin-registry.h |    8 ++-
>  2 files changed, 166 insertions(+), 71 deletions(-)
> 
> diff --git a/src/grl-plugin-registry.c b/src/grl-plugin-registry.c
> index eca8b86..cdebebf 100644
> --- a/src/grl-plugin-registry.c
> +++ b/src/grl-plugin-registry.c
> @@ -61,6 +61,13 @@ GRL_LOG_DOMAIN(plugin_registry_log_domain);
>                                 GRL_TYPE_PLUGIN_REGISTRY,        \
>                                 GrlPluginRegistryPrivate))
>  
> +/* GQuark-like implementation, where we manually assign the first IDs. */
> +struct IDHandler {
> +  GHashTable *string_to_id;
> +  GArray *id_to_string;
> +  gint last_id;
> +};

I'd rename IDHandler to KeyIDHandler, id alone is too generic to get its
true purpose without looking at the rest of the code (plugins also have
IDs for example).

I would rename the internal APIs to use that naming convention as well.
(...)

> +/**
> + * id_handler_add:
> + * @handler: the handler
> + * @key: a specific key for system keys, or GRL_METADATA_KEY_INVALID for it to
> + * be assigned
                  ^^ I would add "automatically" at the end.
> + * @name: the name of the key.
> + *
> + * Add a new key<->name correspondence.
> + *
> + * Returns: the new key number, or GRL_METADATA_KEY_INVALID if the key could
> + * not be created (typically if @key or @name is already registered).
> + */
> +static GrlKeyID
> +id_handler_add (struct IDHandler *handler, GrlKeyID key, const gchar *name)
> +{
> +  GrlKeyID _key = key;
> +
> +  if (_key == GRL_METADATA_KEY_INVALID) {
> +    /* existing keys go from 1 to (id_to_string->len - 1), so the next
> +     * available key is id_to_string->len, which will be incremented by
> +     * g_array_insert_val() */
> +    _key = handler->id_to_string->len;
> +  }
> +
> +  if (NULL != id_handler_get_name (handler, _key)) {
> +    GRL_WARNING ("Cannot register %d:%s because key is already defined as %s",
> +                 _key, name, id_handler_get_name (handler, _key));
> +    return GRL_METADATA_KEY_INVALID;
> +  } else if ( GRL_METADATA_KEY_INVALID != id_handler_get_key (handler, name)) {
                ^ blank space

> +    /* _key or name is already in use! */
> +    GRL_WARNING ("Cannot register %d:%s because name is already registered with key %d",
> +                 _key, name, id_handler_get_key (handler, name));
> +    return GRL_METADATA_KEY_INVALID;
> +  } else {
> +    /* name_copy is shared between handler->id_to_string and
> +     * handler->string_to_id */
> +    gchar *name_copy = g_strdup (name);
> +
> +    if (_key >= handler->id_to_string->len)
> +      g_array_set_size (handler->id_to_string, _key + 1);
> +
> +    /* yes, g_array_index() is a macro that give you an lvalue */
                                                   ^^ missing 's'

> +    g_array_index (handler->id_to_string, const gchar *, _key) = name_copy;
> +    g_hash_table_insert (handler->string_to_id,
> +                         name_copy, GRLKEYID_TO_POINTER (_key));
> +  }
> +
> +  return _key;
> +
> +}