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@xxxxxxxxx>
>
> ---
> 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
Attachment:
pgpK7aUhYBTWr.pgp
Description: PGP signature
_______________________________________________ Libosinfo mailing list Libosinfo@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libosinfo
