Created attachment 116660 [details] [review]
possible solution of problems mentioned in comment 0

ChangeLog:

2008-08-15  Szilárd Pfeiffer  <szilard.pfeiffer@gmail.com>

    * glib/src/nodetree.hg: Implemented copy contructor. Wrapped function
    g_node_copy and g_node_copy_deep. Fixed memory leak in case of the 
    destructor. Fixed parametering problem of member function unlink().
    Bug #547901

There are maybe some comments missing in the code to explain obvious questions such as:

a) Why doesn't copy_deep() just use g_node_copy_deep()?
b) Why do we need copy_deep() if we have a copy constructor that does the same thing? Or doesn't that do a deep copy? I think it should.
c) Why would we want the copy() function that shares the underlying data pointers? Isn't that just an invitation to memory problems?

The new destructor implementation should also have a comment saying why it doesn't just use g_node_destroy(). 

Created attachment 117227 [details] [review]
modified patch which considers the questions in comment 2

a) copy_deep must construct the C++ wrapper, g_node_copy_deep cannot do that
b) constructors copies the data now (originally I want to avoid that, but it would cause much more serious problems)
c) copy not wrapped (it cannot be, because constructors always copy the data)
d) destructor implementation is commented

If the patch looks good and you merge it, please consider removing the comment of the related test case.

Thanks, but:

(In reply to comment #3)
> Created an attachment (id=117227) [edit]
> modified patch which considers the questions in comment 2
> 
> a) copy_deep must construct the C++ wrapper, g_node_copy_deep cannot do that

I don't see a comment in the constructor about g_node_copy_deep(). 

I also don't understand why you have added a copy_deep() method. Why shouldn't people just use the copy constructor.

Also, if there is a copy constructor, surely there should be an operator=()?, with them both maybe sharing some code.

> b) constructors copies the data now (originally I want to avoid that, but it
> would cause much more serious problems)

That sounds safer. Thanks.

> c) copy not wrapped (it cannot be, because constructors always copy the data)
> d) destructor implementation is commented
> 
> If the patch looks good and you merge it, please consider removing the comment
> of the related test case.

Feel free to change the test case in your patch.

(In reply to comment #4)
> Thanks, but:
> 
> (In reply to comment #3)
> > Created an attachment (id=117227) [edit]
> > modified patch which considers the questions in comment 2
> > 
> > a) copy_deep must construct the C++ wrapper, g_node_copy_deep cannot do that
> 
> I don't see a comment in the constructor about g_node_copy_deep().
I put the comment inside the copy_deep() function.
 
> 
> I also don't understand why you have added a copy_deep() method. Why shouldn't
> people just use the copy constructor.
They can use both of them. If somebody like the C-like  version he can use copy_deep(), if he likes the C++ version he can use the copy constructor.

> 
> Also, if there is a copy constructor, surely there should be an operator=()?,
> with them both maybe sharing some code.
> 
Which operator= is on your mind?

 - operator=(const T& data)
   it is not a big deal, but the functionality can be acquired by data() function
 - operator=(const NodeTree<T>& node)
  What should it mean? Should it change parent/children/next/prev pointers, or only overwites the data with the copy of node.data()? In my humble opinion it might be the source of misunderstandings.

Created attachment 117378 [details] [review]
modified patch related to comment 5

Thanks. I have committed that. However:

> > I also don't understand why you have added a copy_deep() method. Why shouldn't
> > people just use the copy constructor.
> They can use both of them. If somebody like the C-like  version he can use
> copy_deep(), if he likes the C++ version he can use the copy constructor.

That doesn't seem sensible. It's a C++ API. Having two ways to do it just clutters the API and gives the impression that one way does something different. I removed copy_deep().

> > Also, if there is a copy constructor, surely there should be an operator=()?,
> > with them both maybe sharing some code.
> > 
> Which operator= is on your mind?
> 
>  - operator=(const T& data)
>    it is not a big deal, but the functionality can be acquired by data()
> function
>  - operator=(const NodeTree<T>& node)

This one.

>   What should it mean? Should it change parent/children/next/prev pointers, or
> only overwites the data with the copy of node.data()? In my humble opinion it
> might be the source of misunderstandings.

In general, I think that any copy constructor (not all constructors) should have a corresponding operator=() because it's not always easy to know when one or the other will be used, so they should both exist, and should both do the same thing. I have added that.

I think that's everything, but please reopen if necessary.

Thanks for your work and attention on this.

(In reply to comment #7)
> 
> In general, I think that any copy constructor (not all constructors) should
> have a corresponding operator=() because it's not always easy to know when one
> or the other will be used, so they should both exist, and should both do the
> same thing. I have added that.

In general, I really agree with you, but it is a little bit special case.

In this case there is a difference between the copy constructor and the operator= in my mind. The copy constructor creates a brand new NodeTree object which is a root item, so it has neither parent nor any siblings.

In case of operator= the object in question may be in a tree, so it may have parent and siblings. If we want to retain the original position of the object in the tree we must not unlink it (as it happens in your implementation).

The another problem with the implementation of the operator= is the fact that it does not copy the data as the copy constructor does.

(In reply to comment #8)
> In this case there is a difference between the copy constructor and the
> operator= in my mind. The copy constructor creates a brand new NodeTree object
> which is a root item, so it has neither parent nor any siblings.
> 
> In case of operator= the object in question may be in a tree, so it may have
> parent and siblings. If we want to retain the original position of the object
> in the tree we must not unlink it (as it happens in your implementation).

That's weird. I guess we need to not affect the other tree, while creating a new one.

> The another problem with the implementation of the operator= is the fact that
> it does not copy the data as the copy constructor does.

Oops.

Could you patch it to do that?

(In reply to comment #9)
> (In reply to comment #8)
> > In this case there is a difference between the copy constructor and the
> > operator= in my mind. The copy constructor creates a brand new NodeTree object
> > which is a root item, so it has neither parent nor any siblings.
> > 
> > In case of operator= the object in question may be in a tree, so it may have
> > parent and siblings. If we want to retain the original position of the object
> > in the tree we must not unlink it (as it happens in your implementation).
> 
> That's weird. I guess we need to not affect the other tree, while creating a
> new one.

I do not understand what kind of "other tree" you have mentioned. The problem is that the clear function what you have created from the destructor unlinks the node (if it is not the root) which the operator= function belongs to.

> 
> > The another problem with the implementation of the operator= is the fact that
> > it does not copy the data as the copy constructor does.
> 
> Oops.
> 
> Could you patch it to do that?
> 
Yes, I could.

> > > In case of operator= the object in question may be in a tree, so it may have
> > > parent and siblings. If we want to retain the original position of the object
> > > in the tree we must not unlink it (as it happens in your implementation).
> > 
> > That's weird. I guess we need to not affect the other tree, while creating a
> > new one.
> 
> I do not understand what kind of "other tree" you have mentioned. The problem
> is that the clear function what you have created from the destructor unlinks
> the node (if it is not the root) which the operator= function belongs to.

Yes, so we should not do that.

For instance, after:

1: node_tree other = get_some_node_tree_from_somewhere()
2: node_tree child = other->get_some_child_node();
3: child = get_some_diferent_node();

other should be the same after step 3 as at step 1.

Created attachment 117799 [details] [review]
patch which reflects to comment 10

ChangeLog:

2008-08-26  Szilárd Pfeiffer  <szilard.pfeiffer@gmail.com>

        * glib/src/nodetree.hg: Implemented clone function to merge the 
        constructors into that and fixed clear function the operator= 
        function.
        * tests/glibmm_nodetree/main.cc: Simplified the test case.
        Bug #547901.

Committed. Thanks for being so responsive and thanks for making this class solid.