[Libosinfo] [v2 2/8] Add enum list param getter

[Christophe Fergeau]

Same nit about the commit log, I couldn't even try to guess what this patch
would be about just by reading the log (no mention of 'entity' there for
example).

On Sun, Feb 10, 2013 at 06:41:03PM +0200, Zeeshan Ali (Khattak) wrote:
> From: "Zeeshan Ali (Khattak)" <zeeshanak at gnome.org>
> 
> ---
>  osinfo/libosinfo.syms  |  1 +
>  osinfo/osinfo_entity.c | 71 ++++++++++++++++++++++++++++++++++++++++----------
>  osinfo/osinfo_entity.h |  4 +++
>  3 files changed, 62 insertions(+), 14 deletions(-)
> 
> diff --git a/osinfo/libosinfo.syms b/osinfo/libosinfo.syms
> index 41d3756..1ad4d82 100644
> --- a/osinfo/libosinfo.syms
> +++ b/osinfo/libosinfo.syms
> @@ -403,6 +403,7 @@ LIBOSINFO_0.2.4 {
>      global:
>  	osinfo_device_driver_format_get_type;
>  	osinfo_device_driver_get_format;
> +	osinfo_entity_get_param_value_enum_list;
>  } LIBOSINFO_0.2.3;
>  
>  /* Symbols in next release...
> diff --git a/osinfo/osinfo_entity.c b/osinfo/osinfo_entity.c
> index 543c710..8b9286e 100644
> --- a/osinfo/osinfo_entity.c
> +++ b/osinfo/osinfo_entity.c
> @@ -382,24 +382,19 @@ gint osinfo_entity_get_param_value_enum(OsinfoEntity *entity,
>                                          GType enum_type,
>                                          gint default_value)
>  {
> -    const gchar *nick;
> -    GEnumClass *enum_class;
> -    GEnumValue *enum_value;
> +    GList *value_list;
> +    gint ret;
>  
>      g_return_val_if_fail(G_TYPE_IS_ENUM(enum_type), default_value);
>  
> -    nick = osinfo_entity_get_param_value(entity, key);
> -    if (nick == NULL)
> -        return default_value;
> -
> -    enum_class = g_type_class_ref(enum_type);
> -    enum_value = g_enum_get_value_by_nick(enum_class, nick);
> -    g_type_class_unref(enum_class);
> -
> -    if (enum_value != NULL)
> -        return enum_value->value;
> +    value_list = osinfo_entity_get_param_value_enum_list(entity,
> +                                                         key,
> +                                                         enum_type,
> +                                                         default_value);
> +    ret = GPOINTER_TO_INT(value_list->data);
> +    g_list_free (value_list);
>  
> -    g_return_val_if_reached(default_value);
> +    return ret;

This is awkward, the code would be nicer if there was an internal
'enum_str_to_int' helper that was shared by this function and
get_param_value_enum_list (though I'm about to contradict myself down this
mail ;)

>  }
>  
>  /**
> @@ -425,6 +420,54 @@ GList *osinfo_entity_get_param_value_list(OsinfoEntity *entity, const gchar *key
>      return g_list_copy(values);
>  }
>  
> +/**
> + * osinfo_entity_get_param_value_enum_list:
> + * @entity: an #OsinfoEntity containing the parameters
> + * @key: the name of the key
> + *
> + * Retrieve all the parameter values associated with a named key as enums. If
> + * no values are associated, a list with only @default_value is returned.
> + *
> + * Returns: (transfer container) (element-type gint): the values associated with the key
> + */
> +GList *osinfo_entity_get_param_value_enum_list(OsinfoEntity *entity,
> +                                               const char *key,
> +                                               GType enum_type,
> +                                               gint default_value)
> +{
> +    GList *value_list;
> +    GList *iter;
> +    GList *ret = NULL;
> +    GList *default_list;
> +
> +    default_list = g_list_append (NULL, GINT_TO_POINTER(default_value));

I'd tend to do return g_list_append (NULL, GINT_TO_POINTER(default_value));
where needed to avoid creating and destroying an unneeded list when
everything works fine. This could alternatively be wrapped in a
#define DEFAULT_LIST g_list_append (NULL, GINT_TO_POINTER(default_value))
if you prefer.

> +
> +    g_return_val_if_fail(G_TYPE_IS_ENUM(enum_type), NULL);

We should return 'default_list' rather than NULL here.

> +
> +    value_list = osinfo_entity_get_param_value_list(entity, key);
> +    if (value_list == NULL)
> +        return default_list;
> +
> +    for (iter = value_list; iter; iter = iter->next) {
> +        GEnumClass *enum_class;
> +        GEnumValue *enum_value;
> +        const gchar *nick = (const gchar *) iter->data;
> +
> +        enum_class = g_type_class_ref(enum_type);
> +        enum_value = g_enum_get_value_by_nick(enum_class, nick);
> +        g_type_class_unref(enum_class);
> +
> +        if (enum_value != NULL)
> +            ret = g_list_append(ret,  GINT_TO_POINTER(enum_value->value));
> +    }

This could be:

GEnumClass *enum_class;
GEnumValue *enum_value;

enum_class = g_type_class_ref(enum_type);
for (iter = value_list; iter; iter = iter->next) {
       const gchar *nick = (const gchar *) iter->data;

       enum_value = g_enum_get_value_by_nick(enum_class, nick);

       if (enum_value != NULL)
           ret = g_list_append(ret,  GINT_TO_POINTER(enum_value->value));
}
g_type_class_unref(enum_class);

(which does not go well with my helper suggestion above).
Also, when enum_value is NULL, we should warn that an unexpected value was
found, and print the value. We could also add the default_value to the list
when this happens, but I don't think this is right.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libosinfo/attachments/20130213/ace7730b/attachment.sig>