Created attachment 188905 [details] [review]
The wrap statement patch

Created attachment 188906 [details]
The generated dbusconnection.h file

Created attachment 188907 [details]
The generated dbusconnection.cc file

This sounds incredibly useful, though I don't have time to review the patch right now.

Note that this information will come from the gobject-introspection files in future, though that's no reason to block this. Note also that we will sometimes still want the other version of the method to have a different name, such as unset_*().

Okay.  As far as methods that should use a different name than the original (such as unset_*()), looking at the gtkmm sources, almost all of these methods have no parameters and are written as complements to a set_*() method wrapped from a C function that accepts a NULL to clear what has been set (unset_icon() in Gtk::Entry takes one parameter with a default value).  I guess prepending an "un" to an override's name that has no parameter could work.

Some unset_*() methods in Gtk::Widget are written as complements to some override_*() methods (for example unset_font() unsets the font set with override_font()).  In these cases maybe an optional parameter specifying the desired name for a parameterless method would be better though maybe unoverride_*() would work as well.

At any rate, I have committed the patch because it does not change gmmproc's functionality and adds a little more flexibility when wrapping C functions with optional parameters.

Thanks.

It would be nice if you also mentioned that optional syntax in the appendix in the gtkmm book, in the part about _WRAP_METHOD().

This should later commit should probably be converted because NULL should never appear in C++ code. (void*)0 might be more appropriate.

Author: José Alburquerque <jaalburqu@svn.gnome.org>
Date:   Wed Jun 1 23:28:14 2011 -0400

    gmmproc: Use NULL for optional properties.
    
        * tools/pm/Output.pm(get_ctor_properties): Use NULL instead of
        static_cast<char*>(0) for properties that are optional as is done in
        convert_args_cpp_to_c().

(In reply to comment #8)
> This should later commit should probably be converted

I mean, reverted.

Is it okay to use simply 0 instead of NULL?  I fixed the last commit using just 0 and things work fine when using optional properties and parameters.  Please let me know if I really should use (void*)0 for NULL properties (problems arise when using (void*)0 for optional parameters).

I'm in the process of making it possible to reorder parameters by using the remapping syntax mentioned above except that instead of ending a parameter name with {num[?]} to remap from the C++ parameter to a specific C parameter in the C function, it will be possible to use {c_name[?]} to remap from the C++ parameter to a C parameter by name instead of a C parameter number (the [?] means an optional ? which signifies that the parameter is optional).  Also, I think I'm pretty close to have an optional output parameter.  If I should use (void*)0 somewhere I'll include that fix in these changes.  After, I'll add the comments about the new syntax.

(In reply to comment #10)
> Is it okay to use simply 0 instead of NULL? 

Yes. that should be fine here.

> I'm in the process of making it possible to reorder parameters by using the
> remapping syntax mentioned above except that instead of ending a parameter name
> with {num[?]} to remap from the C++ parameter to a specific C parameter in the
> C function, it will be possible to use {c_name[?]} to remap from the C++
> parameter to a C parameter by name instead of a C parameter number (the [?]
> means an optional ? which signifies that the parameter is optional).  Also, I
> think I'm pretty close to have an optional output parameter.  If I should use
> (void*)0 somewhere I'll include that fix in these changes.  After, I'll add the
> comments about the new syntax.

That all sounds very useful. Thanks.

There's one final addition that might be possible and I'm wondering if it would be worth adding also.  It might be possible for _WRAP_METHOD to allow the method to return a value constructed from an output parameter of C the function.  For example, in gstreamermm the Gst::Message*::parse*() methods could be wrapped instead of handwritten.  This might be possible by allowing _WRAP_METHOD to "omit" certain parameters and having the method return a value constructed from an output parameter of the C function call.

Actually, I think I've spent a little more time than I should on this when there may be more important things to do.  This last feature I'll leave for later after I've worked on other things.  I'll add the docs on the new functionality next, leaving that last feature for later when there is time.

Created attachment 199306 [details] [review]
Patch to allow setting parameters from C output parameters

I'm attaching this patch because I think it's mature enough to be looked at.  It make things like the following possible when trying to wrap, for example, g_dbus_connection_call_with_unix_fd_list_finish[1] among other similar functions (see an existing TODO in giomm's dbusconnection.hg):

[1] http://developer.gnome.org/gio/2.30/GDBusConnection.html#g-dbus-connection-call-with-unix-fd-list-finish

#m4 _INITIALIZATION(`Glib::RefPtr<UnixFDList>&',`GUnixFDList*', `$3 = Glib::wrap($4);')
  _WRAP_METHOD(Glib::VariantContainerBase call_finish(const Glib::RefPtr<AsyncResult>& res{.}, Glib::RefPtr<UnixFDList>& out_fd_list{.@}), g_dbus_connection_call_with_unix_fd_list_finish, errthrow)

What it does is that it allows setting C++ parameters from C output variables (The '@' following the 'out_fd_list' parameter name tags it as a parameter that should be set from the C paramter which is a C output parameter.  The generated code would look like:

Glib::VariantContainerBase Connection::call_finish(const Glib::RefPtr<AsyncResult>& res, Glib::RefPtr<UnixFDList>& out_fd_list)
{
  GError* gerror = 0;
  GUnixFDList* g_out_fd_list;

  Glib::VariantContainerBase retvalue = Glib::VariantContainerBase(g_dbus_connection_call_with_unix_fd_list_finish(gobj(), &g_out_fd_list, Glib::unwrap(res), 0), false);
  out_fd_list = Glib::wrap(g_out_fd_list);


  if(gerror)
    ::Glib::Error::throw_exception(gerror);

  return retvalue;
}

I'm comfortable enough to say it works.  What might be different is whether '@' should be used to signal such functionality (that's what was easiest at the moment).

I just wanted to post it so that it can be assessed.  In the next few days I'll test it thoroughly, modify the output parameter signaling if it should be modified and then post back with the results so hopefully this patch can be accepted.

Created attachment 199616 [details] [review]
Updated Patch to allow setting parameters from C output parameters

I've updated the patch so that the syntax for signaling that a C++ parameter should be set from a C output variable would be something like:

#m4 _INITIALIZATION(`Glib::RefPtr<UnixFDList>&',`GUnixFDList*', `$3 = Glib::wrap($4)')
  _WRAP_METHOD(Glib::VariantContainerBase call_finish(const Glib::RefPtr<AsyncResult>& res{.}, Glib::RefPtr<UnixFDList>& out_fd_list{.>>}), g_dbus_connection_call_with_unix_fd_list_finish, errthrow)

Where the '>>' in the '{}' following the 'out_fd_list' parameter name means that that variable should be set from a C output parameter.  The generated code would look like:

Glib::VariantContainerBase Connection::call_finish(const Glib::RefPtr<AsyncResult>& res, Glib::RefPtr<UnixFDList>& out_fd_list)
{
  GError* gerror = 0;
  GUnixFDList* g_out_fd_list = 0;
  Glib::VariantContainerBase retvalue = Glib::VariantContainerBase(g_dbus_connection_call_with_unix_fd_list_finish(gobj(), &g_out_fd_list, Glib::unwrap(res), &(gerror)), false);

  if(gerror)
    ::Glib::Error::throw_exception(gerror);

  out_fd_list = Glib::wrap(g_out_fd_list);
  return retvalue;
}

Which is different from the code posted before because the previous patch was preliminary and this one corrects any mistakes I could find and enhances the previous one.

The results of the tests follow. Keep in mind that these test are just to ensure that the code generated by using the syntax is correct.  The examples themselves may not make sense.  For each example, there will be the gmmproc syntax followed by the code that it generates:

----------------------------------------------------------------------------
1) A static void method:

#m4 _INITIALIZATION(`Glib::RefPtr<Gdk::Screen>&',`GdkScreen',`$3 = Glib::wrap($4)')
  _WRAP_METHOD(static void remove_provider_for_screen(Glib::RefPtr<Gdk::Screen>& screen{>>}, const Glib::RefPtr<StyleProvider>& provider), gtk_style_context_remove_provider_for_screen)

void StyleContext::remove_provider_for_screen(Glib::RefPtr<Gdk::Screen>& screen, const Glib::RefPtr<StyleProvider>& provider)
{
  GdkScreen g_screen = 0;
  gtk_style_context_remove_provider_for_screen(&g_screen, Glib::unwrap(provider));
  screen = Glib::wrap(g_screen);
}
----------------------------------------------------------------------------
2) A static void method throwing a Glib::Error:

(See the comment after the rest of the tests)
----------------------------------------------------------------------------
3) A static non-void method:

#m4 _INITIALIZATION(`TimeZone&',`GTimeZone',`$3 = Glib::wrap($4)')
  _WRAP_METHOD(static DateTime create(TimeZone& tz{>>}, int year, int month, int day, int hour, int minute, double seconds), g_date_time_new)

DateTime DateTime::create(TimeZone& tz, int year, int month, int day, int hour, int minute, double seconds)
{
  GTimeZone g_tz = 0;
  DateTime retval = Glib::wrap(g_date_time_new(&g_tz, year, month, day, hour, minute, seconds));
  tz = Glib::wrap(g_tz);
  return retval;
}
----------------------------------------------------------------------------
4) A static non-void method where the C return is placed in an output parameter:

#m4 _INITIALIZATION(`TimeZone&',`GTimeZone',`$3 = Glib::wrap($4)')
#m4 _INITIALIZATION(`DateTime&',`GDateTime*',`$3 = Glib::wrap($4)')
  _WRAP_METHOD(static void create(DateTime& output{OUT}, TimeZone& tz{>>}, int year, int month, int day, int hour, int minute, double seconds), g_date_time_new)

void DateTime::create(DateTime& output, TimeZone& tz, int year, int month, int day, int hour, int minute, double seconds)
{
  GTimeZone g_tz = 0;
  output = Glib::wrap(g_date_time_new(&g_tz, year, month, day, hour, minute, seconds));
  tz = Glib::wrap(g_tz);
}
----------------------------------------------------------------------------
5) A static non-void method throwing a Glib::Error:

#m4 _INITIALIZATION(`bool&',`gboolean',`$3 = $4')
  _WRAP_METHOD(static bool check_replacement(const Glib::ustring& replacement, bool& has_references{>>}), g_regex_check_replacement, errthrow)

bool Regex::check_replacement(const Glib::ustring& replacement, bool& has_references)
{
  GError* gerror = 0;
  gboolean g_has_references = 0;
  bool retvalue = g_regex_check_replacement(replacement.c_str(), &g_has_references, &(gerror));

  if(gerror)
    ::Glib::Error::throw_exception(gerror);

  has_references = g_has_references;
  return retvalue;
}
----------------------------------------------------------------------------
6) A static non-void method throwing a Glib::Error where the C return is placed in an output parameter:

#m4 _INITIALIZATION(`bool&',`gboolean',`$3 = $4')
  _WRAP_METHOD(static void check_replacement(bool& output{OUT}, const Glib::ustring& replacement, bool& has_references{>>}), g_regex_check_replacement, errthrow)

void Regex::check_replacement(bool& output, const Glib::ustring& replacement, bool& has_references)
{
  GError* gerror = 0;
  gboolean g_has_references = 0;
  output = g_regex_check_replacement(replacement.c_str(), &g_has_references, &(gerror));
  if(gerror)
    ::Glib::Error::throw_exception(gerror);

  has_references = g_has_references;
}
----------------------------------------------------------------------------
7) A non-static void method:

#m4 _INITIALIZATION(`Glib::ustring&',`const gchar',`$3 = $4')
  _WRAP_METHOD(void change_action_state(Glib::ustring& action_name{>>}, const Glib::VariantBase& value), g_action_group_change_action_state)

void ActionGroup::change_action_state(Glib::ustring& action_name, const Glib::VariantBase& value)
{
  const gchar g_action_name = 0;
  g_action_group_change_action_state(gobj(), &g_action_name, const_cast<GVariant*>((value).gobj()));
  action_name = g_action_name;

}
----------------------------------------------------------------------------
8) A non-static void method throwing a Glib::Error:

#m4 _INITIALIZATION(`Glib::RefPtr<SocketAddress>&',`GSocketAddress',`$3 = $4')
  _WRAP_METHOD(void bind(Glib::RefPtr<SocketAddress>& address{>>}, bool allow_reuse), g_socket_bind, errthrow)

void Socket::bind(Glib::RefPtr<SocketAddress>& address, bool allow_reuse)
{
  GError* gerror = 0;
  GSocketAddress g_address = 0;
  g_socket_bind(gobj(), &g_address, static_cast<int>(allow_reuse), &(gerror));

  if(gerror)
    ::Glib::Error::throw_exception(gerror);

  address = g_address;

}
----------------------------------------------------------------------------
9) A non-static non-void method:

#m4 _INITIALIZATION(`Glib::RefPtr<IOStream>&',`GIOStream', `$3 = Glib::wrap(&($4))')
  _WRAP_METHOD(bool authorize_authenticated_peer(Glib::RefPtr<IOStream>& stream{>>}, const Glib::RefPtr<const Credentials>& credentials), g_dbus_auth_observer_authorize_authenticated_peer)

bool AuthObserver::authorize_authenticated_peer(Glib::RefPtr<IOStream>& stream, const Glib::RefPtr<const Credentials>& credentials)
{
  GIOStream g_stream = 0;
  bool retvalue = g_dbus_auth_observer_authorize_authenticated_peer(gobj(), &g_stream, const_cast<GCredentials*>(Glib::unwrap<Gio::Credentials>(credentials)));
  stream = Glib::wrap(&(g_stream));
  return retvalue;

}
----------------------------------------------------------------------------
10) A non-static non-void method where the C return is placed in an output parameter:

#m4 _INITIALIZATION(`Glib::RefPtr<IOStream>&',`GIOStream', `$3 = Glib::wrap(&($4))')
#m4 _INITIALIZATION(`bool&',`gboolean', `$3 = $4')
  _WRAP_METHOD(void authorize_authenticated_peer(bool& output{OUT}, Glib::RefPtr<IOStream>& stream{>>}, const Glib::RefPtr<const Credentials>& credentials), g_dbus_auth_observer_authorize_authenticated_peer)

void AuthObserver::authorize_authenticated_peer(bool& output, Glib::RefPtr<IOStream>& stream, const Glib::RefPtr<const Credentials>& credentials)
{
  GIOStream g_stream = 0;
  output = g_dbus_auth_observer_authorize_authenticated_peer(gobj(), &g_stream, const_cast<GCredentials*>(Glib::unwrap<Gio::Credentials>(credentials)));
  stream = Glib::wrap(&(g_stream));

}
----------------------------------------------------------------------------
11) A non-static non-void method throwing a Glib::Error:

#m4 _INITIALIZATION(`Glib::RefPtr<UnixFDList>&',`GUnixFDList*', `$3 = Glib::wrap($4)')
  _WRAP_METHOD(Glib::VariantContainerBase call_finish(const Glib::RefPtr<AsyncResult>& res{.}, Glib::RefPtr<UnixFDList>& out_fd_list{.>>}), g_dbus_connection_call_with_unix_fd_list_finish, errthrow)

Glib::VariantContainerBase Connection::call_finish(const Glib::RefPtr<AsyncResult>& res, Glib::RefPtr<UnixFDList>& out_fd_list)
{
  GError* gerror = 0;
  GUnixFDList* g_out_fd_list = 0;
  Glib::VariantContainerBase retvalue = Glib::VariantContainerBase(g_dbus_connection_call_with_unix_fd_list_finish(gobj(), &g_out_fd_list, Glib::unwrap(res), &(gerror)), false);

  if(gerror)
    ::Glib::Error::throw_exception(gerror);

  out_fd_list = Glib::wrap(g_out_fd_list);
  return retvalue;
}
----------------------------------------------------------------------------
12) A non-static non-void method throwing a Glib::Error where the C return is placed in an output param (modified from above example):

#m4 _INITIALIZATION(`Glib::RefPtr<UnixFDList>&',`GUnixFDList*', `$3 = Glib::wrap($4)')
#m4 _INITIALIZATION(`Glib::VariantContainerBase&',`GVariant*', `$3 = Glib::wrap($4)')
  _WRAP_METHOD(void call_finish(Glib::VariantContainerBase& output{OUT}, const Glib::RefPtr<AsyncResult>& res{.}, Glib::RefPtr<UnixFDList>& out_fd_list{.>>}), g_dbus_connection_call_with_unix_fd_list_finish, errthrow)

void Connection::call_finish(Glib::VariantContainerBase& output, const Glib::RefPtr<AsyncResult>& res, Glib::RefPtr<UnixFDList>& out_fd_list)
{
  GError* gerror = 0;
  GUnixFDList* g_out_fd_list = 0;
  output = Glib::wrap(g_dbus_connection_call_with_unix_fd_list_finish(gobj(), &g_out_fd_list, Glib::unwrap(res), &(gerror)));

  if(gerror)
    ::Glib::Error::throw_exception(gerror);

  out_fd_list = Glib::wrap(g_out_fd_list);

}
----------------------------------------------------------------------------

I could not find a static void method throwing a Glib::Error but that is very similar to the first test (modulo the Glib::Error).  The Glib::Error handling is handled by the Output::convert_args_cpp_to_c() which always generates conversions (and with this patch declarations for the C output parameters and initializations to set the C++ parameters from the C output parameters).  Since it has not failed in any of the other tests, I'm confident that this last case would also be successful.

I'm submitting this patch because I think it will help in future wrapping, that's all.  I completely understand if it looks like it may not be useful but I think the patch will be useful.

(In reply to comment #15)
> #m4 _INITIALIZATION(`Glib::RefPtr<UnixFDList>&',`GUnixFDList*', `$3 =
> Glib::wrap($4)')
> #m4 _INITIALIZATION(`Glib::VariantContainerBase&',`GVariant*', `$3 =
> Glib::wrap($4)')

Could that _INITIALIZATION have a prefix, please, even if it is just GMMPROC. I know our other ones don't have that, but let's not keep doing bad things. 

This also feels like a backwards _CONVERSION(), which is a bit odd.

>  _WRAP_METHOD(Glib::VariantContainerBase call_finish(const
Glib::RefPtr<AsyncResult>& res{.}, Glib::RefPtr<UnixFDList>& out_fd_list{.>>}),
g_dbus_connection_call_with_unix_fd_list_finish, errthrow)

> Where the '>>' in the '{}' following the 'out_fd_list' parameter name means
> that that variable should be set from a C output parameter.  The generated code
> would look like:

I find that >> really obscure. Can we have something clearer? And how does that differ to OUT?

(In reply to comment #16)
> (In reply to comment #15)
> > #m4 _INITIALIZATION(`Glib::RefPtr<UnixFDList>&',`GUnixFDList*', `$3 =
> > Glib::wrap($4)')
> > #m4 _INITIALIZATION(`Glib::VariantContainerBase&',`GVariant*', `$3 =
> > Glib::wrap($4)')
> 
> Could that _INITIALIZATION have a prefix, please, even if it is just GMMPROC. I
> know our other ones don't have that, but let's not keep doing bad things.

Would '_MM_' be okay?

> 
> This also feels like a backwards _CONVERSION(), which is a bit odd.

I understand.  If the functionality is worth it, maybe it can be better documented.  I think there's something in the gtkmm-documentation module, but maybe I can add a little more to clarify if that helps.

In essence, the macro tells gmmproc how to set a C++ type from a C type.  '$3' in the third argument represents the name of the C++ parameter while '$4' represents the C value.

> 
> >  _WRAP_METHOD(Glib::VariantContainerBase call_finish(const
> Glib::RefPtr<AsyncResult>& res{.}, Glib::RefPtr<UnixFDList>& out_fd_list{.>>}),
> g_dbus_connection_call_with_unix_fd_list_finish, errthrow)
> 
> > Where the '>>' in the '{}' following the 'out_fd_list' parameter name means
> > that that variable should be set from a C output parameter.  The generated code
> > would look like:
> 
> I find that >> really obscure. Can we have something clearer? And how does that
> differ to OUT?

I guess so.  Maybe using a different symbol or something like that.  Would you have a preference?  It's different from OUT in that if a parameter in the method declaration is appended with {OUT} the return of the C function will be used to set that parameter.

On the other hand, a parameter appended with '>>' (or a better representation) means that the C function has a parameter that it sets (in which a value is returned) and the C++ parameter should be set from the C parameter whifch stores the value that the C function has set.  With the patch, gmmproc would generate a declaration of a temporary variable in which store the value that the C function sets and pass the variable by C reference to the function.  After, the C++ parameter is set from the temporary variable.

I didn't mention, but it may be possible to duplicate this functionality for wrapping virtual functions which I have found that sometimes require setting the C++ parameter from a variable that the C function receives by reference and then sets.

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > >  _WRAP_METHOD(Glib::VariantContainerBase call_finish(const
> > Glib::RefPtr<AsyncResult>& res{.}, Glib::RefPtr<UnixFDList>& out_fd_list{.>>}),
> > g_dbus_connection_call_with_unix_fd_list_finish, errthrow)
> > 
> > > Where the '>>' in the '{}' following the 'out_fd_list' parameter name means
> > > that that variable should be set from a C output parameter.  The generated code
> > > would look like:
> > 
> > I find that >> really obscure. Can we have something clearer? And how does that
> > differ to OUT?
> 
> I guess so.  Maybe using a different symbol or something like that.  Would you
> have a preference?  It's different from OUT in that if a parameter in the
> method declaration is appended with {OUT} the return of the C function will be
> used to set that parameter.
> 
> On the other hand, a parameter appended with '>>' (or a better representation)
> means that the C function has a parameter that it sets (in which a value is
> returned) and the C++ parameter should be set from the C parameter whifch
> stores the value that the C function has set.  With the patch, gmmproc would
> generate a declaration of a temporary variable in which store the value that
> the C function sets and pass the variable by C reference to the function. 
> After, the C++ parameter is set from the temporary variable.

Maybe using {RET} for a C++ parameter that should be set from the C function return as before and using {OUT} for the C++ parameters that should be set from variables that the C function sets would be better.  The only problem with that is that it is possible to map a C++ parameter to a C parameter by including the name of the C parameter in the '{}' (this is so that it is possible to order the parameters of the method declaration as desired).  an 'OUT' somewhere in the' {}' where a C parameter name might be also, might be a little confusing.

> 
> I didn't mention, but it may be possible to duplicate this functionality for
> wrapping virtual functions which I have found that sometimes require setting
> the C++ parameter from a variable that the C function receives by reference and
> then sets.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glibmm/issues/10.