GNOME Bugzilla – Bug 724732
add() methods hide virtual Container::add()
Last modified: 2014-07-13 13:44:51 UTC
When compiling some GUI applications of our robot software Fawkes (http://www.fawkesrobotics.org) with Gtkmm 3.10 (on Fedora 20) with clang++ 3.3 fails due to overloaded virtual functions: In file included from /usr/include/gtkmm-3.0/gtkmm.h:247: /usr/include/gtkmm-3.0/gtkmm/stack.h:124:8: error: 'Gtk::Stack::add' hides overloaded virtual function [-Werror,-Woverloaded-virtual] void add(Widget& child, const Glib::ustring& name); ^ /usr/include/gtkmm-3.0/gtkmm/container.h:165:15: note: hidden overloaded virtual function 'Gtk::Container::add' declared here: different number of parameters (1 vs 2) virtual void add(Widget& widget); From a glance at the master in the git repo it seems the problem still exists.
Is this only a problem when explicitly using the -Woverloaded-virtual option? I have built gtkmm with clang fairly recently. Of course, I agree that this is a bad method override.
We use -Wall which seems to include -Woverloaded-virtual. Since we also use -Werror having such problems in third party libs is kind of annoying. We worked around it by disablig this particular warning for stuff that uses Gtkmm. But I'd like to see this fixed upstream.
OK. I also use clang and scan-build exactly in order to get as many warnings as possible so I can fix them. Maybe you'd like to add a new method with an improved name, deprecating the old one. Then you might avoid the clang warning in your application's build by disabling deprecated API.u
What do you mean, you want me to send in a patch for Gtkmm with a new method?
Unfortunately gcc's -Wall does not turn on -Woverloaded-virtual. I tested with gcc and the option -Woverloaded-virtual. It generates a warning. When I added using Container::add; in stack.hg, Container::add() is not hidden. No warning! Does the using-declaration break ABI? Does it change the vtable? I guess that it does not, but it's just a guess. I guess that the vtable contains all virtual functions of the base classes, even hidden functions.
(In reply to comment #4) > What do you mean, you want me to send in a patch for Gtkmm with a new method? Yes, a patch would be nice, please.
(In reply to comment #5) > When I added > using Container::add; > in stack.hg, Container::add() is not hidden. No warning! Sorry, I don't understand how this should fix the problem. Could you explain more?
The -Woverloaded-virtual option is misnamed (at least in gcc, I don't know if that also goes for clang++). It does not warn if a virtual function is overloaded. It warns if a virtual function is hidden. The gcc docs say "Warn when a function declaration hides virtual functions from a base class." If you include "using Container::add;" in the definition of Gtk::Stack, the virtual function Container::add() is not hidden by the declarations of add() overloads in Stack. All three add() functions are available in Stack, virtual void add(Widget& widget), inherited from Container, void add(Widget& child, const Glib::ustring& name) and void add(Widget& child, const Glib::ustring& name, const Glib::ustring& title), introduced in Stack.
(In reply to comment #3) > Maybe you'd like to add a new method with an improved name, deprecating the old > one. Then you might avoid the clang warning in your application's build by > disabling deprecated API. Actually, that's probably not a good idea. Container::add() is only virtual because we neded to override it in ScrolledWindow::add() but, now that GTK+ has been fixed, we no longer need to do that. I've added a TODO about that: So we can remove the virtual from Container::add() at the next ABI break. If we added add_whatever() methods to work around this problem now, we'd just have to remove that API in future and bring back the deprecated add() methods(). So I guess we have to live with this for now. https://git.gnome.org/browse/gtkmm/commit/?id=d95b163fd367cfb1cb397ed225aef9a33ada84a9
Without "using Container::add" Stack::add() will hide Container::add() even if Container::add() is not virtual. Is that what we want? Perhaps it's unimportant. Container::add() is probably not very useful for Stack objects. It would add a nameless child widget. On the other hand, would it do any harm to make Container::add() visible in Stack? Except that I'm not 100% sure that it won't change Stack's vtable, and thus break ABI, when Container::add() is virtual.
(In reply to comment #10) > Without "using Container::add" Stack::add() will hide Container::add() even > if Container::add() is not virtual. Is that what we want? Well, hiding a virtual method seems more important than hiding a non-virtual method. Would hiding a non virtual method cause any warnings? Where exactly would this "using" go? Maybe you could upload a patch to make it absolutely clear to me, please.
Created attachment 271245 [details] [review] patch: Gtk::Stack: Don't hide Container::add() Here's a patch. gcc with the -Woverloaded-virtual option does not warn if a non-virtual add() method is hidden in Stack.
So this would be the same as adding a Stack::add() that called Container::add()? Tim, could you please tell us if the patch would fix the problem for you with clang.
The API would be the same with using Container::add; as with void Stack::add(Widget& widget) { Container::add(widget); } but the ABI would be different. The using-declaration does not define or declare a Stack::add() function, it makes Container::add() visible in Stack, just as it would have been if there had been no Stack::add() functions. Stack::add(Widget& widget) adds a Stack::add() that overrides the virtual Container::add(Widget& widget). That Stack::add() function shows up in the object code. (I checked with the nm command.) See e.g. Bjarne Stroustrup's C++ book, 3rd edition, 15.2.2, or 4th edition, 20.3.5.
I think I'd prefer to just add void Stack::add(Widget& widget) { Container::add(widget); } because it's simpler and more obvious to most people. I don't think that would break any existing API or ABI.
Though I wonder if it would be a runtime error to have a stack child widget with no name.
> I think I'd prefer to just add > void Stack::add(Widget& widget) { Container::add(widget); } > because it's simpler and more obvious to most people. I don't mind. > I don't think that would break any existing API or ABI. Neither do I. > Though I wonder if it would be a runtime error to have a stack child widget > with no name. I haven't tested, but I don't think it's an error. https://git.gnome.org/browse/gtk+/tree/tests/teststack.c contains gtk_container_add (GTK_CONTAINER (stack), w2); gtk_container_child_set (GTK_CONTAINER (stack), w2, "name", "2", "title", "2", "needs-attention", TRUE, NULL); Not much of a proof, when a name is set immediately after the call to gtk_container_add(), but the code in gtkstack.c always tests for name == NULL before name is used.
Created attachment 276943 [details] [review] patch: Gtk::Stack: Don't hide Container::add() Here's an alternative to the patch in commant 12. gcc with the -Woverloaded-virtual option does not warn if this patch is applied. What shall we do with this bug? Let it remain open until Tim or someone else confirms that one of the patches makes it possible to compile with clang++? Push one of the patches anyway?
I have tested both patches (comment 12 and comment 18) with clang++ 3.4. Both patches make it possible to compile with -Wall -Werror. I have pushed a patch similar to the one in comment 18. I marked Stack::add(Widget& child) both new and deprecated. It's not meant to be used. https://git.gnome.org/browse/gtkmm/commit/?id=fff186e79ad6d8be7a4528c526369bd17ab45006