GNOME Bugzilla – Bug 690134
Gio::Action::get_state() API is broken
Last modified: 2013-08-06 19:14:18 UTC
Created attachment 231423 [details] [review] Action: Fix API for get_state() and get_state_hint() Right now Gio::Action::get_state() and get_state_hint() return void instead of a Glib::VariantBase. Since these functions are useless in their current form and its doubtful anyone used it, is it ok to change the API? Attached is the patch that would fix it.
Yes, these functions are useless, as well as a memory leak, when they return void. The change of API should not be a problem. An application program that calls get_state() or get_state_hint() without using a returned value will compile also after the change. But I suppose this is also a change of ABI. That can be more problematic. Assume an application program calls get_state() and it's compiled with a version of glibmm where get_state() returns void. Then the glibmm library is upgraded to a version where get_state() returns Glib::VariantBase. The application is _not_ recompiled. Will it still work as before? Does anyone know? It's a pity we can't overload two functions where only the type of the return value differs. We can add new functions with other names, but it would be ugly.
Unfortunately I do get a segfault using a pre-patch compiled version against a post-patch library. The stack is probably ending up different than the following code expects.
I made this mistake and I don't even know how. I'm very sorry. The only workaround I can think of is to use the C functions g_action_get_[state|state_hint]() with Gio::Action::gobj() and to wrap the returned GVaraint in a Glib::VaraintBase until it's possible to fix this bug when an ABI break is possible.
We can add member functions with other names, e.g. _WRAP_METHOD(Glib::VariantBase get_state_hint2() const, g_action_get_state_hint) _WRAP_METHOD(Glib::VariantBase get_state2() const, g_action_get_state) and add documentation that says they will be deprecated at the next ABI break. Would it be too ugly? Are there better names than xxx2? The alternative is to add documentation to get_state() and get_state_hint(), showing exactly what code is required to call the C functions and wrap the returned GVariant in a Glib::VariantBase.
Might also want to consider the state is available through the GObject property. (But the state_hint isn't)
I have changed the documentation of get_state_hint() and get_state(), and described what to do instead of calling those methods. I suppose we'd better leave it like that until the next ABI break. I keep this bug open until it can be properly fixed.
I can't image any application that would be calling get_state() if it returns void, so I feel like this would not be an ABI break. Nobody would call such a useless message.
Created attachment 250300 [details] [review] patch: Gio::Action: Fix API for get_state() and get_state_hint(). So you mean that I can push this updated version of Andrew's patch now?
Well, I can't see who it would hurt, but maybe I've misunderstood something above.
I have pushed an updated version of the patch in comment 8. (It didn't apply because of other changes in action.hg after I made the patch.) Action::get_state() and get_state_hint() are then fixed, but I noticed other suspicious code in action.cc, some copied from action.ccg, some generated by gmmproc. 1. Memory leak bool Action::get_state_bool() const { g_return_val_if_fail( g_action_get_state_type(const_cast<GAction*>(gobj())) == G_VARIANT_TYPE_BOOLEAN, false); GVariant* state = g_action_get_state(const_cast<GAction*>(gobj())); g_return_val_if_fail(state, false); return g_variant_get_boolean(state); } Needs a call to g_variant_unref(state), if state != 0. const bool result = g_variant_get_boolean(state); g_variant_unref(state); return result; 2. Possible accesses to deallocated memory. Glib::VariantType Action::get_parameter_type() const { return Glib::wrap(const_cast<GVariantType*>(g_action_get_parameter_type( const_cast<GAction*>(gobj()))), false); } Glib::VariantType Action::get_state_type() const { return Glib::wrap(const_cast<GVariantType*>(g_action_get_state_type( const_cast<GAction*>(gobj()))), false); } Glib::wrap() must take a copy of GVariantType. variant.hg contains // It's necessary to take an extra reference of the 'const GVariantType*' // returned by g_variant_get_type() because it doesn't do that already. #m4 _CONVERSION(`const GVariantType*',`VariantType', `Glib::wrap(const_cast<GVariantType*>($3), true)') action.hg needs // It's necessary to take an extra reference of the 'const GVariantType*' // returned by g_action_get_state_type() and get_parameter_type() because they // don't do that already. #m4 _CONVERSION(`const GVariantType*',`Glib::VariantType', `Glib::wrap(const_cast<GVariantType*>($3), true)') or change _CONVERSION(`const GVariantType*',`Glib::VariantType', `Glib::wrap(const_cast<GVariantType*>($3), false)') in tools/m4/convert_gio.m4. Such a change may affect other classes. It's probably the right thing to do, but it needs more investigation.
Thanks. Please go ahead and fix things where you are sure.
I have pushed a patch with the changes I discussed in comment 10. https://git.gnome.org/browse/glibmm/commit/?id=566548222c2bb53661a7d9717f916e5ef07a224d I found still more memory leaks. They are described in the new bug 705124. Closing this bug now.
I've improved this further by making the methods templated: https://git.gnome.org/browse/glibmm/commit/?id=14a000d4a7e2e3312bae25bdc5cc8e0062b63314 Let me know if that template implementation doesn't work for some types, or if this otherwise seems wrong.