[patch] generate marshallers and metadata from dbus-glib-tool
Havoc Pennington
hp at redhat.com
Tue Aug 24 13:57:55 PDT 2004
Hi,
Woohoo! Nice work. This is really helpful.
Some comments of varying importance:
- + g_assert (c_name != NULL);
probably not safe unless the parser ensures it... but the parser
should probably allow c_name to be omitted since the parser is also
used at runtime for introspection, where c_name isn't relevant. So
a g_printerr/exit on this might be better
- I generally avoid declaration/assignment mixing, so
char *foo = NULL;
vs.
char *foo;
foo = NULL;
- if only one OUT arg is supported for now, it'd be good to put a clean
error handling in for that - though in the end we should support
multiple OUT I think, with the first one used as the C return.
- looking at the patch it's hard for me to tell what the generated code
looks like, so there may be some comments there but I have to poke
at it more
- it seems like there must be a way to avoid the release/cleanup
function, maybe include it in the marshaller or just use knowledge of
the types involved... ?
- it looks like each marshaller probably has some of the same code;
if this could be factored out it'd save code size. Ideally the
marshallers only contain the code that varies by the function
signature.
- it would probably be good to avoid include dbus-glib-lowlevel
and maybe this is achievable by factoring out the common
code from the marshallers, and/or by changing the GLib bindings
to use GLib data types in the varargs list ... ?
- I see now that the parser forces c_name, I'm thinking we should
avoid this for the introspection case
- release_func should probably be called c_release_func, though
again it'd be nice to find a way to avoid this
Just explain to me in small words if I'm confused on any of this, I'm
optimizing for fast reply to your mail rather than detailed thinking
through the patch ;-)
Thanks,
Havoc
More information about the dbus
mailing list