couple gtktreedatalist.c comments

[Tim Janik <timj gtk org>]

hey jrb,

i saw a few issues while browsing you union storage stuff in gtktreedatalist.c:

/* node allocation
 */
struct _GAllocator /* from gmem.c */
{
  gchar           *name;
  guint16          n_preallocs;
  guint            is_unused : 1;
  guint            type : 4;
  GAllocator      *last;
  GMemChunk       *mem_chunk;
  GtkTreeDataList *free_nodes;
};


G_LOCK_DEFINE_STATIC (current_allocator);
static GAllocator *current_allocator = NULL;

do _not_ do this,
1) GAllocator is private in GLib for a reason
2) GAllocator is just there for the glib functions that support it
3) will become obsolete pretty soon anyways

smply use a memchunk like all other code does as well and
be done with it.

void
_gtk_tree_data_list_free (GtkTreeDataList *list,
                          GType           *column_headers)
{
  GtkTreeDataList *tmp;
  gint i = 0;

  for (tmp = list; tmp; tmp = tmp->next)
    {
      switch (column_headers [i])
        {
        case G_TYPE_STRING:
          g_free ((gchar *) tmp->data.v_pointer);
          break;
        case G_TYPE_OBJECT:
          g_object_unref (G_OBJECT (tmp->data.v_pointer));
          break;
        default:
          break;
        }
      i++;
    }

  G_LOCK (current_allocator);
  list->next = current_allocator->free_nodes;
  current_allocator->free_nodes = list;
  G_UNLOCK (current_allocator);
}

i didn't investigate too deep, but aren't you missing boxed here?
also, you have to switch on G_TYPE_FUNDAMENTAL(column_headers [i])
or you can forget about any kind of derived type (GTK_TYPE_IDENTIFIER,
G_TYPE_CLOSURE, GTK_TYPE_WIDGET...) for columns.

void
_gtk_tree_data_list_value_to_node (GtkTreeDataList *list,
                                   GValue          *value)
{
  switch (value->g_type)
    {
    case G_TYPE_BOOLEAN:
      list->data.v_int = g_value_get_boolean (value);
      break;
    case G_TYPE_CHAR:
      list->data.v_char = g_value_get_char (value);
      break;
    case G_TYPE_UCHAR:
      list->data.v_uchar = g_value_get_uchar (value);
      break;
    case G_TYPE_INT:
      list->data.v_int = g_value_get_int (value);
      break;
    case G_TYPE_UINT:
      list->data.v_uint = g_value_get_uint (value);
      break;
    case G_TYPE_POINTER:
      list->data.v_pointer = g_value_get_pointer (value);
      break;
    case G_TYPE_FLOAT:
      list->data.v_float = g_value_get_float (value);
      break;
    case G_TYPE_STRING:
      list->data.v_pointer = g_value_dup_string (value);
      break;
    default:
      if (g_type_is_a (G_VALUE_TYPE (value), G_TYPE_OBJECT))
        list->data.v_pointer = g_value_dup_object (value);
      else if (g_type_is_a (G_VALUE_TYPE (value), G_TYPE_BOXED))
        list->data.v_pointer = g_value_dup_boxed (value);
      else
        g_warning ("Unsupported type (%s) stored.", g_type_name (value->g_type));
      break;
    }
}

since GValue*value is supplied by the user, it should be const,
and you definitely need to switch on G_TYPE_FUNDAMENTAL (G_VALUE_TYPE(value)),
user values will get passed in as GTK_TYPE_WIDGET...
also, value->g_type is _private_, just use G_VALUE_TYPE() and nothing
else to figure a values type.
also, it'd be good to support G_TYPE_DOUBLE as well, for model frontends
that e.g. use vararg interfaces, all values are supplied as double (default
promotion in ANSI C) there's no good reason to enfore those to do lossage
prone conversions to float due to a limitation in the model.
also, G_TYPE_OBJECT and G_TYPE_BOXED are fundamental types, that means
they are enum IDs and you can directly switch() on them.

as a side note, though you won't need that after you switch on fundamental IDs,
you can use G_VALUE_HOLDS (value, GTK_TYPE_WIDGET) instead of the is_a checks.
for the fundamentals, there are even compund macros G_VALUE_HOLDS_BOXED(value)
etc...


upon fnction entries that take values, such as:

void
gtk_list_store_set_cell (GtkListStore *list_store,
                         GtkTreeIter  *iter,
                         gint          column,
                         GValue       *value)
{
  GtkTreeDataList *list;
  GtkTreeDataList *prev;
  GtkTreePath *path;

  g_return_if_fail (list_store != NULL);
  g_return_if_fail (GTK_IS_LIST_STORE (list_store));
  g_return_if_fail (iter != NULL);
  g_return_if_fail (column >= 0 && column < list_store->n_columns);

  prev = list = G_SLIST (iter->user_data)->data;

  while (list != NULL)
    {
      if (column == 0)
        {
          path = gtk_list_store_get_path (GTK_TREE_MODEL (list_store), iter);
          _gtk_tree_data_list_value_to_node (list, value);


you should put:
g_return_if_fail (G_IS_VALUE (value));
unless you also know the type that you expect, e.g. for string you can use
g_return_if_fail (G_VALUE_HOLDS_STRING (value));
right away, the macros will deal with NULL values, they are there to be
used in return_if_fail statements.


ok, that's it for the moment, i desperately need sleep ;)


---
ciaoTJ