totally rewritten glib bindings

Havoc Pennington hp at redhat.com
Mon Feb 14 19:01:23 PST 2005


Hi,

Bunch of nitpicks, since I read the patch anyhow...

you should add -p to your diff options so function names show.

> g_string_new_len ("", 0);

g_string_new(NULL) is the same as this I think

dbus-gidl.h needs egtk-format-protos (probably wait until you switch to
annotations)

> +extern gboolean dbus_glib_output_c (BaseInfo *info, GIOChannel
*channel, GError **error);

should put this in a header file so there's only one copy of it

In dbus-gobject.c, dbus-gidl.h shouldn't be included, unless we cleaned
up gidl a lot to be suitable for shared lib usage (e.g. namespace)

dbus-marshal-basic.h won't do you any good since the symbols in there
aren't exported from libdbus (assuming libtool is in a good mood at
least). I'm guessing this is for DBusBasicValue... we should probably
just declare a local 8-byte struct to use, or move DBusBasicValue to
dbus-shared.h (that would make it public though, so maybe dbus-shared-
internal, I dunno)

> +      if (!dbus_gvalue_demarshal (&iter, &value))
> +        {
> +          g_warning ("Unable to convert arg type %d to GValue",
dtype);
> +	  g_value_array_free (ret);
> +	  ret = NULL;

Indentation funkiness (probably just \t characters I guess, M-x
untabify)

lookup_object_and_method() should also look at the interface (in case
two methods have the same name; note that NULL interface is allowed in
the message, in which case we just pick the first method we see)

I'm guessing timj will get unhappy if he finds out we're putting
GClosure on the stack, but it does seem more efficient.

All of this code will break completely when we support container types.
Then the count of GValue won't match the number of bytes in the
signature, etc. That's OK but we should probably fail in a non-segfaulty
way if someone hands us a container.

Objects should be stored in GValue as G_TYPE_OBJECT or
G_TYPE_FROM_INSTANCE (object) rather than pointer

> +#include <locale.h>
> +#include <libintl.h>
> +#define _(x) dgettext (GETTEXT_PACKAGE, x)
> +#define N_(x) x

can be g18n.h now except for the dgettext

Havoc




More information about the dbus mailing list