Re: couple gtktreedatalist.c comments

[Tim Janik <timj gtk org>]

On 9 Mar 2001 jrb redhat com wrote:

> > 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...
> 
> It's not necessarily supplied by the user -- it's normally generated by
> gtk_list_store_set... but point taken.  I removed the switch statement
> and am using g_type_is_a () instead.  Are the G_VALUE_HOLDS functions a
> better idea?

yes, they check !=NULL as well.

> Additionally, I added G_TYPE_DOUBLE this afternoon.

great.

> > you should put:
> > g_return_if_fail (G_IS_VALUE (value));
> 
> I'm testing for NULL here, but you're right.

ok, i'm just saying G_IS_VALUE(value) gives you that check _and_ assures
the value's type s actually a valid value type (meaning it has a GTypeValueTable
and is not flagged as VALUE_ABSTRACT).

> > 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.
> 
> I used:
>         g_return_if_fail (g_type_is_a (G_VALUE_TYPE (type), list_store->column_headers[column]));
> 
> again here.  Is that Okay?

well, that's ok if you checked for value!=NULL already, but
g_return_if_fail (G_VALUE_HOLDS (value, list_store->column_headers[column]));
is a bit more compact right? (and does !=NULL as well)

> If you have a few seconds to look at the new stuff, that would be great.

err, i just updated and gtktreedatalist.c still contains GAllocator code,
but i'll comment throguh the file anyways:

gboolean
_gtk_tree_data_list_check_type (GType type)
{
  gint i = 0;

  static GType type_list[] =
  {
    G_TYPE_BOOLEAN,
    G_TYPE_CHAR,
    G_TYPE_UCHAR,
    G_TYPE_INT,
    G_TYPE_UINT,
    G_TYPE_ENUM,
    G_TYPE_FLAGS,
    G_TYPE_FLOAT,
    G_TYPE_DOUBLE,
    G_TYPE_STRING,
    G_TYPE_POINTER,
    G_TYPE_BOXED,
    G_TYPE_OBJECT,
    G_TYPE_INVALID
  };

  while (type_list[i] != G_TYPE_INVALID)
    {
      if (g_type_is_a (type, type_list[i]))
        return TRUE;
      i++;
    }
  return FALSE;
}
i didn't check what exactly you intended this function for, probably
to check column types. just be aware, that while you can do:
g_value_init(&value, G_TYPE_CLOSURE); /* G_TYPE_CLOSURE is_a G_TYPE_BOXED */
you can not:
g_value_init(&value, G_TYPE_BOXED);
that's because G_TYPE_BOXED, G_TYPE_ENUM and G_TYPE_FLAGS are flagged as
VALUE_ABSTRACT, that means they implement a GTypeValueTable, but you
need to actually use derived types thereof for actuall values (that's because
you can't e.g. free() a boxed pointer without knowing its exact type).
so if you want to cover real usable value types, you'd use
G_TYPE_IS_VALUE_TYPE() which returns has_value_table && !VALUE_ABSTRACT for
the given type.

void
_gtk_tree_data_list_node_to_value (GtkTreeDataList *list,
                                   GType            type,
                                   GValue          *value)
{
  g_value_init (value, type);

  if (g_type_is_a (type, G_TYPE_BOOLEAN))
    g_value_set_boolean (value, (gboolean) list->data.v_int);
  else if (g_type_is_a (type, G_TYPE_CHAR))

this you can majorly optimize by simply doing:

switch (G_TYPE_FUNDAMENTAL (type))
{
case G_TYPE_BOOLEAN:
  g_value_set_boolean (value, (gboolean) list->data.v_int);
  break;

because you only care about fundamental types there. the same applies to
_gtk_tree_data_list_value_to_node() and _gtk_tree_data_list_node_copy().

tell me what else files you need reviewed.

> 
> Thanks,
> -Jonathan
> 

---
ciaoTJ