[patch] generate marshallers and metadata from dbus-glib-tool

[Paul Kuliniewicz]

On Tue, Aug 24, 2004 at 04:57:55PM -0400, Havoc Pennington wrote:
> Hi,
> 
> Woohoo! Nice work. This is really helpful.

Thanks!

> 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

As you noted, I made c_name required, though this can be changed.
Proper error behavior would probably be to g_printerr and skip that
method.

> - I generally avoid declaration/assignment mixing, so 
>   char *foo = NULL;
>   vs.
>   char *foo;
>   foo = NULL;

No problem.

> - 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.

Right; right now it more or less blindly assumes that's the case.  If we
want to support multiple OUT arguments, we'll need to stop sorting the
argument list based on direction.  Plus, I suppose we'd need to handle
IN/OUT args, which would be treated separately at the D-BUS level but as
a single argument at the GLib level.

How would you specify a (somewhat pathological) function that returns
void but whose first argument is OUT?

> - 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

The generator could use some comments, certainly.  I've attached the
generated code you get from the example I sent with the patch.  It
doesn't actually look too bad, if you don't mind the line lengths.

> - 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... ?

This could get tricky.  The main issue is with functions that return
strings.  How can the marshaller know if the string is statically
allocated (in which case it mustn't be freed) or dynamically allocated
(in which case it must)?  GLib uses the convention that a const char *
return shouldn't be freed and a char * return should, but D-BUS's types
don't make that distinction.  And, of course, there's the problem of
what happens if the object doesn't follow that convention.  So, at the
very least, you need to flag which OUT arguments need to be freed.

Also, the marshaller needs to know how to free any dynamically allocated
results.  Calling g_free() is probably the most common case, but it's
certainly possible to have a library that uses its own alloc/free
functions for strings (like, for example, D-BUS's dbus_malloc() and
dbus_free()).  Using the wrong function could end up putting a library
in an inconsistent state.

Hard-coding knowledge of how to release strings would be possible if you
assumed g_free() is always the right thing to do, though it would
require adding a notional DBUS_TYPE_STATIC_STRING type within the
marshaller generator to make the distinction.  If you need to support a
user-specified release function, you need an extra type for each such
function, which can quickly get out of hand (especially if you have
multiple OUT parameters!).

Interestingly, the current design of the patch also allows you to
specify a release function for *any* OUT argument, even things like
ints.  I can't think of a case where this would actually be useful, but
I'm sure someone out there could think of something. :-)

> - 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's the same basic code in each case, but a lot of it depends on the
particular types of the arguments being marshalled.  The handler for
what happens if dbus_message_get_args() fails could be pulled out pretty
easily.  The code that builds the reply message could also probably be
refactored a bit to eliminate the duplication of error handling.

> - 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 think dbus-glib-lowlevel.h is only needed for
dbus_g_connection_get_connection() and dbus_g_message_get_message().  If
the non-G versions of these structures are passed to the marshaller
instead, the conversion can go away, along with the #include.

> - I see now that the parser forces c_name, I'm thinking we should 
>   avoid this for the introspection case

Right.

> - release_func should probably be called c_release_func, though
>   again it'd be nice to find a way to avoid this

Fair enough, though I still think it needs to be specified somehow, and
at the very least you need to flag which OUT arguments need to be freed.

> 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 ;-)

You mean you can't skim through a patch that adds lots of uncommented C
code that generates more C code with several ugly typecasts and immediately
figure out everything you need to know? ;-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://freedesktop.org/pipermail/dbus/attachments/20040824/c18f965b/attachment.pgp