[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