GNOME Bugzilla – Bug 511136
TreeView::get_cursor() may return an invalid TreePath instance
Last modified: 2012-04-10 12:26:01 UTC
TreeView::get_cursor "returns" a TreePath, which according to the C documentation, may be a NULL pointer (the gtkmm documenation is quiet about this). The only way to check if the path is valid that I can see is checking if path.gobj() is valid, which is (i) ugly and (ii) undocumented. My suggestion would be to implement TreePath::operator bool () const { return gobj() != NULL; } or at least update the documentation to explain how the validity of a returned TreePath can be checked. The returned TreeViewColumn is probably affected as well. Other information:
Isn't TreePath::empty() enough?
TreePath::empty() does work, but it will throw the following error message: Gtk-CRITICAL **: gtk_tree_path_get_depth: assertion `path != NULL' failed The issue here is that get_cursor doesn't return an empty TreePath, it returns a NULL-pointer TreePath, so all of the gtk_tree_path_* functions will produce the above error message.
Ah. I'd happily accept a patch against svn trunk that did the extra check in empty() and also added an operator bool() that called empty(). As a workaround in the meantime, I guess you should indeed use if(treepath.gobj())
I'd still like a patch.
Fixed in svn trunk.
I think this change is broken and a bad idea. It makes no sense to make empty() work on a TreePath which is essentially in a state it should never have got into in the first place. The remainder of the TreePath methods would still fail anyway. The correct fix is to change get_cursor() to treat the NULL case specially, just as we already do for many string return values. On a side note, I don't think it is a good idea to add operator bool() to classes all over the place. It will make any instance implicitly convertible to integer type.
(In reply to comment #6) > I think this change is broken and a bad idea. It makes no sense to make empty() > work on a TreePath which is essentially in a state it should never have got > into in the first place. It's debatable whether it should be in that state. > The remainder of the TreePath methods would still fail > anyway. But checking with operator bool can avoid that. We use the same technique elsewhere already, at least in pangomm, I think. > The correct fix is to change get_cursor() to treat the NULL case specially, > just as we already do for many string return values. But we can't be sure that we won't get a NULL GtkTreePath* from someplace else, so I'd like to keep this operator bool. I'm fine with you also adding a get_cursor() method overload with a bool& parameter, assuming that's what you mean.
(In reply to comment #7) > > work on a TreePath which is essentially in a state it should never have got > > into in the first place. > > It's debatable whether it should be in that state. TreePath does not, in any way, represent a pointer or pointer-like data structure. It isn't a handle. The interface is that of an STL container. Also, there is no need to make a distinction between an invalid TreePath and an empty one. > > > The remainder of the TreePath methods would still fail > > anyway. > > But checking with operator bool can avoid that. So we end up with two ways to represent an empty TreePath. One is actually empty but valid, the other isn't a valid TreePath at all. As far as empty() is concerned, they are now the same thing. But they suddenly aren't the same thing anymore if you use some other method on it. Why make empty() work, but not size() == 0, or begin() == end(), or... In other words: If the intent is to represent the state "no cursor position" as an empty TreePath, then why not actually make it an empty TreePath? Why invent an additional state "in limbo"? The case with nstring in gtkmm 1.2 was much less clear-cut, and we still got rid of it. > We use the same technique elsewhere already, at least in pangomm, I think. I hope these usages actually introduce a distinct state, and not just an alternative internal representation for one and the same logical state. > > The correct fix is to change get_cursor() to treat the NULL case specially, > > just as we already do for many string return values. > > But we can't be sure that we won't get a NULL GtkTreePath* from someplace else, so I'd like to keep this operator bool. If operator bool() weren't identical to empty() but would actually indicate a distinct state, it would be acceptable. That is, it would be acceptable if we actually needed to somehow represent a state distinct from an empty path. But we don't, as far as I'm aware. > I'm fine with you also adding a > get_cursor() method overload with a bool& parameter, assuming that's what you > mean. Eeek, no. Output parameters are a horrible last resort. And having such an overload *in addition* to some other means to represent that state is just ugly.
> TreePath does not, in any way, represent a pointer or pointer-like data > structure. It isn't a handle. The interface is that of an STL container. Also, > there is no need to make a distinction between an invalid TreePath and an empty > one. We don't make a distinction. We do say that a (internally) invalid one is as useless as an empty one. As far as I know there is know, and empty TreePath has no meaning other than, "no path". A std::vector can be empty too. It doesn't have operator bool() for that. Are you saying that you'd be happy with just making empty() check for an null gobj()? > Eeek, no. Output parameters are a horrible last resort. And having such an > overload *in addition* to some other means to represent that state is just > ugly. I'm not sure anymore what you actually want.
(In reply to comment #9) > We don't make a distinction. We do say that a (internally) invalid one is as > useless as an empty one. As far as I know there is know, and empty TreePath has > no meaning other than, "no path". But it is still a valid container in one case but not in another. So why have two internal states to say the same thing? > A std::vector can be empty too. It doesn't have operator bool() for that. But std::vector::empty() does not attempt to handle the situation where the vector object is corrupted and in an invalid state. > Are > you saying that you'd be happy with just making empty() check for an null > gobj()? Er, no. That's how it is right now. bool TreePath::empty() const { if(!gobject_) return false; return (gtk_tree_path_get_depth(gobject_) == 0); } > I'm not sure anymore what you actually want. Ditch the check for NULL and operator bool(), and custom-code TreeView::get_cursor() to return an empty but valid path if the GTK+ function returns NULL.
OK. Murray and I discussed the problem on IRC. We agreed on removing operator bool() again, and making sure that the object is never in an invalid state to begin with. However, this should be done in a generic way and therefore needs to go into the constructor. Unfortunately the constructor is auto-generated, and it is not possible to easily override the autogeneration. For this reason this won't be a quick fix, unfortunately, but will have to wait until someone finds the time to add another exception flag to gmmproc to suppress the autogeneration of the constructor. However, the removal of operator bool() doesn't have to wait until then. In fact, it needs to be removed before the API freeze, so I'll do it right now.
(In reply to comment #10) > > Are > > you saying that you'd be happy with just making empty() check for an null > > gobj()? > > Er, no. That's how it is right now. > > bool TreePath::empty() const > { > if(!gobject_) > return false; > > return (gtk_tree_path_get_depth(gobject_) == 0); > } > Bug 586437 actually shows that this code is wrong. If gobjet_ is NULL TreePath::empty() should return 'true'. Bug 586437 has been fixed in trunk.
Yes. It basically means that the questionable check didn't even work correctly.
(In reply to comment #11) > Unfortunately the constructor is auto-generated, and it is not possible to > easily override the autogeneration. For this reason this won't be a quick fix, > unfortunately, but will have to wait until someone finds the time to add > another exception flag to gmmproc to suppress the autogeneration of the > constructor. I think it's easy to fix gmmproc. I just need to know more precisely what you want it not to do. TreePath is declared as _CLASS_BOXEDTYPE(TreePath, GtkTreePath, gtk_tree_path_new, gtk_tree_path_copy, gtk_tree_path_free) The autogenerated constructors are TreePath(); explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true); TreePath(const TreePath& other); The autogeneration of the default constructor can be suppressed by including the m4 macro _CUSTOM_DEFAULT_CTOR in the .hg file. I suppose you want to suppress the autogeneration of explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true); It would be easy to add a macro for that. What about _CUSTOM_WRAP_CTOR? That constructor is used by the 'wrap' function. Unfortunately there is an inconsistency in the way _CUSTOM_xxx macros work in _CLASS_BOXEDTYPE, _CLASS_BOXEDTYPE_STATIC, and _CLASS_OPAQUE_COPYABLE. They suppress both the declaration in the .h file, and the implementation in the .cc file. In _CLASS_GOBJECT and _CLASS_GTKOBJECT, _CUSTOM_xxx means "generate the declaration in the .h file, but suppress the implementation in the .cc file." _NO_xxx means "suppress both declaration and implementation." The same goes for optional arguments custom_xxx and no_xxx in _WRAP_SIGNAL and _WRAP_VFUNC. What shall _CUSTOM_WRAP_CTOR (or whatever it shall be called) do? Shall it suppress both declaration and implementation, or only implementation? Btw, the hand-written constructor explicit TreePath(const Glib::ustring& path); can also construct a TreePath with gobject_ == 0. > However, the removal of operator bool() doesn't have to wait until then. In > fact, it needs to be removed before the API freeze, so I'll do it right now. Was TreePath::operator bool() ever removed? It seems to have been replaced by operator const void*().
(In reply to comment #14) > > However, the removal of operator bool() doesn't have to wait until then. In > > fact, it needs to be removed before the API freeze, so I'll do it right now. > > Was TreePath::operator bool() ever removed? It seems to have been replaced by > operator const void*(). Apparently, no, it wasn't removed. We did, however, change all operator bool()s to operator void*() to avoid unintended conversions. This is apparently correct.
Created attachment 201281 [details] [review] patch: gmmproc: Add macro _DEFAULT_COBJECT_FUNC in _CLASS_BOXEDTYPE (glibmm) What about this solution? In a class of type _CLASS_BOXEDTYPE it's possible to call the new m4 macro _DEFAULT_COBJECT_FUNC that specifies a function that shall be used in constructor explicit CppObjectType(BaseObjectType* gobject, bool make_a_copy = true); to create a new C object, if gobject == 0. It can be called as _DEFAULT_COBJECT_FUNC(NEW) // Use the 3rd parameter of _CLASS_BOXEDTYPE or _DEFAULT_COBJECT_FUNC(function_that_returns_a_BaseObjectType_pointer) If someone can suggest a better name for the new macro, I'm willing to change it. An alternative would be to add an extra optional parameter to the _CLASS_BOXEDTYPE macro.
Created attachment 201282 [details] [review] patch: TreePath: All constructors guarantee that gobject_ != 0. (gtkmm) Use the new m4 macro _DEFAULT_COBJECT_FUNC in treepath.hg. Hand-coded constructors never construct an object with gobject_ == 0. I've also updated .gitignore. Should that be in a separate commit? (In reply to comment #0) > The returned TreeViewColumn is probably affected as well. No, it's not affected in the same way as TreePath. The prototype of TreeView::get_cursor() is void get_cursor(TreeModel::Path& path, TreeViewColumn*& focus_column); i.e. focus_column is a pointer. If no GtkTreeViewColumn is found, get_cursor() returns a null-pointer, which is much better than an invalid object.
Daniel, 2-3 years ago you had a definite opinion about how to modify TreePath. What do you say about the solution I suggest in comment 16 and comment 17? Of course I could add a possibility to suppress the autogeneration of the constructor explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true); and hand-code it, as you discuss in comment 11. My proposed modification of gmmproc instead autogenerates a constructor with the wanted functionality. Perhaps the same modification can be useful in other classes in the future.
(In reply to comment #17) > I've also updated .gitignore. Should that be in a separate commit? Was pushed separately 2012-02-16.
Created attachment 211400 [details] [review] patch: gmmproc: Add macro _CUSTOM_CTOR_CAST in _CLASS_BOXEDTYPE. (glibmm) I've changed my mind. This patch adds _CUSTOM_CTOR_CAST instead of _DEFAULT_COBJECT_FUNC, which I suggested in comment 16. It's more consistent, since there is already a _CUSTOM_CTOR_CAST m4 macro in _CLASS_BOXEDTYPE_STATIC. It's also what's asked for in comment 11. When _CUSTOM_CTOR_CAST is used, gmmproc does not generate an explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true); constructor.
Created attachment 211401 [details] [review] patch: TreePath: All constructors guarantee that gobject_ != 0. (gtkmm) And here's the corresponding gtkmm patch. Use the new m4 macro _CUSTOM_CTOR_CAST in treepath.hg. Hand-code explicit TreePath(GtkTreePath* gobject, bool make_a_copy = true); Hand-coded constructors never construct an object with gobject_ == 0. I'll soon push these patches unless someone objects.
Yes, please.
Actually, I have pushed both. Thanks.