GNOME Bugzilla – Bug 588988
OptionGroup::on_post_parse() overrides must call the base class's implementation
Last modified: 2011-02-10 08:17:16 UTC
Glib::OptionGroup::on_post_parse() require calling inherited. I think this is a design flaw and caused headaches, because the default implementation perform a required step to the option parsing. IMHO Glib::OptionGroup should call its own post_parse callback that in turn WILL call the virtual on_post_parse().
I think this is fairly normal when overriding methods, as long as it's documented. But I'm open to other opinions. If we want to change it, I guess we need to wait for an ABI break.
Requiring to call the inherited implementation is prone to errors. One way would be to have the current implementation not being this virtual method but a another private method that will call the virtual method. I'm sure we can do it in an ABI compatible way, but I don't think it is important enough. However for Gtkmm 3, that would be nice.
A patch would be welcome anyway.
Created attachment 180240 [details] [review] patch: OptionGroup: An on_post_parse() override need not call the base class. Here's the patch. I think it does break ABI, so it's only intended for gtkmm 3. In this patch the C callback function is a static member of Gtk::OptionGroup, thus a function with C++ linkage. I assume that's acceptable. It's the technique used by gmmproc when it generates code for signals and virtual functions. I tried to make the C callback function a friend of class OptionGroup, but it seems a friend function must have C++ linkage and be global, so it would be no better. The only alternative I see is to add a public function, which should only be called by the C callback function. Not an attractive alternative! Other useful modifications of OptionGroup are discussed in bug 589197. (In reply to comment 1) > I think this is fairly normal when overriding methods, as long as it's > documented. I agree, but is it documented?
(In reply to comment #4) > Created an attachment (id=180240) [details] [review] > patch: OptionGroup: An on_post_parse() override need not call the base class. > > Here's the patch. I think it does break ABI, so it's only intended for gtkmm 3. Could you please try creating a patch without the unnecessary change of the callback function name, and without moving that function? Why do you think it breaks ABI? > In this patch the C callback function is a static member of Gtk::OptionGroup, > thus a function with C++ linkage. [snip] Yes, that's fine. I thought I had a bug open that discussed that, with a failed attempt to change it, but I can't find it now.
(In reply to comment #5) > Could you please try creating a patch without the unnecessary change of the > callback function name, and without moving that function? Of course I can keep the old name, but if the callback function shall continue to be a local function in optiongroup.cc with C linkage, then it can only call public methods in class OptionGroup. Then I have to add a public member function to OptionGroup to be able to make OptionGroup::on_post_parse() a dummy function. And that's the whole point of the patch. If the callback function is either a global function with C++ linkage (then it can be a friend of OptionGroup), or a member function of OptionGroup, there's no need for a new public function. But if the callback is given C++ linkage, it will be subjected to C++'s name mangling, and at least the linker will consider its name changed. Is that ok? I don't understand exactly what kind of changes you want to avoid. Or do you want me to move the code from OptionGroup::on_parse_post() to g_callback_post_parse(), and make OptionGroup::CppOptionEntry, type_map_entries, and map_entries public members of OptionGroup, which is necessary if the moved code shall be executed from g_callback_post_parse()? > Why do you think it breaks ABI? I have to confess that I don't understand exactly what counts as an ABI break. I thought that perhaps the new member function that I added, or the old one that I removed would be an ABI break.
Oh, sorry, the last sentence in comment 6 is badly worded. The removed function (g_callback_post_parse) is not a member function. And it can be said more clearly that it's not as simple as just moving some code from OptionGroup::on_post_parse() to g_callback_post_parse(). The code to move uses protected parts of OptionGroup. It can only be executed by a member function or a friend function (and of course a member of a subclass). I have tried to make g_callback_post_parse() a friend of OptionGroup without changing it in any other way, but the compiler (gcc 4.4.5) does not accept any of my attempts.
OK, yes, it needs to be a member function. Adding a (non-virtual) member function will not break ABI. However, I wonder how this will affect code that continues to call the base class method. Will that now be broken?
(In reply to comment #8) > However, I wonder how this will affect code that continues to call the base > class method. Will that now be broken? class CustomOptionGroup : public Glib::OptionGroup { virtual bool on_post_parse(OptionContext& context, OptionGroup& group) { <code block 1> Glib::OptionGroup::on_post_parse(context, group); <code block 2> return true; } ...... }; At <code block 1> there will be a noticeable difference, if the patch in comment 4 is applied. Without the patch: The 'arg' output arguments supplied in the calls to Glib::OptionGroup::add_entry[_filename] have not been updated with the values of the parsed command options. It would even be possible for CustomOptionGroup::on_post_parse to return conditionally before calling Glib::OptionGroup::on_post_parse, deliberately skipping that update. With the patch: The 'arg' arguments are unconditionally updated before CustomOptionGroup::on_post_parse is called. So, yes, it's possible that code will be broken. I haven't thought of this possibility until now. It seems unlikely that anyone would depend on 'arg' not being updated, but it's possible. What do we do? Change only where we can break API? Don't change the code, but document Glib::OptionGroup::on_post_parse, telling what it does, and that it must usually be called from an overriding function? Is API/ABI break permitted in any of the present branches of glibmm? Both in comment 2 and comment 4 gtkmm 3 is mentioned, assuming wrongly that this is gtkmm. I know that the master branch of gtkmm will become gtkmm 3. Will the master branch of glibmm become glibmm 3?
No, we don't plan to do a glibmm ABI break any time soon, though there is a branch where I sometimes gather some changes for the future: http://git.gnome.org/browse/glibmm/log/?h=glibmm-3maybe However, I don't think it's a major problem that the arg would not be updated yet. Let's take the risk, mentioning the change in the NEWS file. Pushed. Thanks.