GNOME Bugzilla – Bug 520778
[PATCH] GNode wrapper implementation
Last modified: 2008-07-29 09:03:39 UTC
glibmm wrapper for glib's GNode.
Created attachment 106695 [details] [review] Patch
I've always been surprised that standard C++ provides no tree container. I wonder if boost or similar has one already. Looking at this quickly, I see some superficial problems: - Don't indent inside the namespace, to be consistent with the rest of glibmm. - Use "Something& " instead of "Something &". - You should use a libsigc++ slot instead of a plain C function pointer. - Your is/get methods should be const. - All member variables should have the _ suffix. - Some method parameters should be const. For instance, position_of(Tree &child). - Tree(gpointer data) should probably use the "explicit" keyword. - There's probably no reason for the destructor to be virtual - there are no virtual methods. - We can should probably use void* instead of gpointer in our public API. - Please avoid use of "using", even in tests.
Created attachment 106714 [details] [review] Updated per comments
Created attachment 107524 [details] [review] Added reinterpret_cast to Tree::*sibling
I've been meaning to take a look at this patch, but haven't gotten around to it yet. I promise I'll try to do it soon. Just wanted to let you know that it's not going to be ignored :)
I don't much like that this container seems to demand that types are used by pointer. For instance, from the test from the patch: std::string *a = new std::string("a"), *b = new std::string("b"), *c = new std::string("c"), *d = new std::string("d"), *e = new std::string("e"), *f = new std::string("f"); Glib::Tree<std::string*> ta(a), tb(b), tc(c), te(e); ta.insert(0, tc); ta.prepend(tb); ta.append_data(d); tc.append(te); te.prepend_data(f); If we can't make it work with values, like std::list, std::vector, etc, then we need a really good reason for that. Otherwise people will feel that it is not really C++ enough.
Created attachment 107926 [details] [review] Updated per comment #6 This will require value types to have a working operator=. Hopefully this isn't too high a price to pay for more C++iness.
Oh, that's much nicer. > This will require value types to have a working operator=. Just as a std container would.
Is this patch against svn? I get several failures when I try to apply it.
Created attachment 107992 [details] [review] Patch against trunk Sorry, I thought I was working against trunk, but I actually had the 2.14 branch. Latest patch is against trunk.
Committed, with some minor documentation changes, and a couple of minor TODOs. Thanks. Please remember to patch the ChangeLog in future.
Created attachment 115050 [details] [review] suggested changes to the wrapper
I would like to suggest some changes in the implementation of the wrapper (comment 12). I can see the following benefits: 1; Glib::Node class would be closer to the original GNode structure, so it would be simpler 2; Foreach or traverse slots should use the tree node instead of the node's data only 3; Data would be compared by the operator == of class T instead of pointer comparison in case of find functions
(In reply to comment #13) > I would like to suggest some changes in the implementation of the wrapper > (comment 12). I can see the following benefits: > > 1; Glib::Node You mean Glib::Tree, I guess. > class would be closer to the original GNode structure, so it > would be simpler > 2; Foreach or traverse slots should use the tree node instead of the node's > data only What would be the advantage of this? > 3; Data would be compared by the operator == of class T instead of pointer > comparison in case of find functions The patch seems to have lots of unrelated changes, which makes it difficult to review.
(In reply to comment #13) > 1; Glib::Node class would be closer to the original GNode structure, so it > would be simpler Are you suggesting renaming Tree to Node? > 3; Data would be compared by the operator == of class T instead of pointer > comparison in case of find functions Data is already compared using the == operator.
(In reply to comment #14) > (In reply to comment #13) > > I would like to suggest some changes in the implementation of the wrapper > > (comment 12). I can see the following benefits: > > > > 1; Glib::Node > > You mean Glib::Tree, I guess. Yes, you are right, otherwise it would be lovely not to name the wrapper of GNode to Glib::Tree especially because there is balanced tree implementatation in Glib with a structure name GTree. > > > class would be closer to the original GNode structure, so it > > would be simpler > > 2; Foreach or traverse slots should use the tree node instead of the node's > > data only > > What would be the advantage of this? In case of the original version (GNode) a GNode * is passed to the callback function, which makes possible to examine the location of the given node in the complete tree (e.g.: depth, index). In case of an own Gtk::TreeModel implementation it is necessary. > > > 3; Data would be compared by the operator == of class T instead of pointer > > comparison in case of find functions > > The patch seems to have lots of unrelated changes, which makes it difficult to > review. >
(In reply to comment #15) > (In reply to comment #13) > > 1; Glib::Node class would be closer to the original GNode structure, so it > > would be simpler > > Are you suggesting renaming Tree to Node? Yes. Sure. (comment 16) > > > 3; Data would be compared by the operator == of class T instead of pointer > > comparison in case of find functions > > Data is already compared using the == operator. > You are right. It is mistake of mine, but with the suggested modifications it is unnecessary to make a copy from the data and delete it in the destructor. explicit Tree(T& data) { T* tmp = new T(); *tmp = data; ... Additionally in case of this kind of implementation the class T must have a constructor with no parameters and you cannot use the Glib::Tree with reference types of data. For example: Glib::Tree<std::string &>
> it would be lovely not to name the wrapper of > GNode to Glib::Tree especially because there is balanced tree implementatation > in Glib with a structure name GTree. Ah. Well spotted. How about using Glib::NodeTree instead, though let's keep that out of any patches for now, to keep things simple. I really don't like the Glib::Node (or GNode) name. It's too obscure. > > > 2; Foreach or traverse slots should use the tree node instead of the node's > > > data only > > > > What would be the advantage of this? > > In case of the original version (GNode) a GNode * is passed to the callback > function, which makes possible to examine the location of the given node in the > complete tree (e.g.: depth, index). In case of an own Gtk::TreeModel > implementation it is necessary. Ah. Could you show this in one of the examples, please. > > The patch seems to have lots of unrelated changes, which makes it difficult to review.
Created attachment 115172 [details] example to comment 18
(In reply to comment #18) > > it would be lovely not to name the wrapper of > > GNode to Glib::Tree especially because there is balanced tree implementatation > > in Glib with a structure name GTree. > > Ah. Well spotted. How about using Glib::NodeTree instead, though let's keep > that out of any patches for now, to keep things simple. I really don't like the > Glib::Node (or GNode) name. It's too obscure. I work with GLib and Glibmm in parallel, so if the name of the wrapper and the original differ it disturbs me, but it is only my humble opinion. It is your choice, but if I have enough time I will write a wrapper to GTree, so I suggest the consideration of the name. > > > > > 2; Foreach or traverse slots should use the tree node instead of the node's > > > > data only > > > > > > What would be the advantage of this? > > > > In case of the original version (GNode) a GNode * is passed to the callback > > function, which makes possible to examine the location of the given node in the > > complete tree (e.g.: depth, index). In case of an own Gtk::TreeModel > > implementation it is necessary. > > Ah. Could you show this in one of the examples, please. > I have given you an exemple (attachment 115172 [details]). It is not the final version, but I hope it shows you my goal. > > > > The patch seems to have lots of unrelated changes, which makes it difficult to review. >
> I work with GLib and Glibmm in parallel, so if the name of the wrapper and the > original differ it disturbs me, but it is only my humble opinion. It is your > choice, but if I have enough time I will write a wrapper to GTree, so I suggest > the consideration of the name. I still don't like the GNode name. And we still need a patch without the unrelated changes. Time is running out, I'm afraid. GNOME API freeze is July 28th. > > > In case of the original version (GNode) a GNode * is passed to the callback > > > function, which makes possible to examine the location of the given node in the > > > complete tree (e.g.: depth, index). In case of an own Gtk::TreeModel > > > implementation it is necessary. > > > > Ah. Could you show this in one of the examples, please. > > > > I have given you an exemple (attachment 115172 [details] [edit]). It is not the final version, > but I hope it shows you my goal. This seems to be a custom Gtk::TreeModel. I guess you mean to show use of methods like is_leaf(), but please try to create a simpler example or test that we can put in glibmm itself.
OK, I finally got some time to look at this API. I think I agree with Szilard that it would be nice to pass a reference to the node to the traversal functions. It doesn't make these functions much more cumbersome and it allows you to do things that you couldn't do otherwise (and it seems to match the C API better as well). The changes proposed also address my main concerns about the initial patch, which is that it exposes too much internal stuff (e.g. the NodeMap typedef) and keeps too much extra stuff in the wrapper class instead of relying on the data from the underlying C object (e.g. the C++ wrapper class had a children_ member that kept track of the node's children separately from the C object that it is wrapping). Regarding the name of the class, I agree that Glib::Tree is a bit problematic considering that there is a totally separate GTree type in Glib. I'm also not a big fan of the name GNode/Glib::Node, but perhaps it would be best to use it anyway just to stay as compatible with glib as possible... Maybe NodeTree is OK, I'm not sure.
Created attachment 115391 [details] [review] Cleaned version of suggested changes I've created the cleaned version of the suggested changes to make the review easier
Thanks. Could you explain the removal of the gobject_, owns_gobject_, children_ and parent_ member variables and the subsequent simplification of the code. Why were they necessary before? Why are they not necessary now? In general, please provide a ChangeLog entry with patches. It should explain what is being changed and why.
I alluded to the removal of the children_ and parent_ in my comment above. Essentially, I believe that the previous code was doing a lot of duplicate book-keeping by keeping references to the children and parents in the C++ wrapper instead of relying on the underlying C object to tell us that information. So I am in favor of removing these and simplifying the wrapper implementation. There's less chance for bugs in the wrapper if it leaves that stuff to the C object.
Sure. Simpler code is better. I just like to see patches explained.
yes, I'd still like to hear szilárd's comments on it as well.
(In reply to comment #24) > Thanks. Could you explain the removal of the gobject_, owns_gobject_, children_ > and parent_ member variables and the subsequent simplification of the code. Why > were they necessary before? Why are they not necessary now? > > In general, please provide a ChangeLog entry with patches. It should explain > what is being changed and why. > gobject_: The member haven't been removed, but it is protected now (should be private, because it's the class own business). It stores the C version (GNode *) of the object as did it before my patch. gobject_->data: Stores this pointer of the C++ object. It makes possible to acquire the C++ version of the object from a C callback. children_: Stored the child nodes of the object. Obsoleted by the idea that the data pointer of the C version stores a pointer to the C++ version, so the GNode * pointers which are returned by the wrapped functions can be "converted" to the C++ version without any map. parent_: Stored a pointer to the parent. It is unnecessary now, because the parent of an object can be acquired via the parent pointer of the C version, just like in case of children. owns_gobject_: Stored whether the node is in a tree. In my humble opinion it is unnecessary at all in case of a wrapper. subsequent changes: Result of the changes which have been mentioned above.
Thanks. I committed the patch, with this ChangeLog entry: 2008-07-29 Szilárd Pfeiffer <szilard.pfeiffer@gmail.com> * glib/src/tree.hg: Make the callbacks take a Tree<> instead of just the data, so they can use methods on the tree (which can be a node in the tree). gobject_: Make this protected. Provide the this pointer as data to g_node_new() so we can retrieve it later. Removed children_ and parent_ because we don't need a separate store now that we can get the C++ instance from the gobject instance. owns_gobject_: Removed because it is was always true, so the gobject was always destroyed (and still is). * tests/glibmm_tree/main.cc: Updated for the changed API. Bug #520778. Please remember to patch the ChangeLog in future.