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

(In reply to comment #0)
> 1; data members (gobject_, data_) should be private instead of protected

Yeah. We don't do this in other parts of glibmm and gtkmm, but maybe we should. gobj() provides access for derived classes.

However, we do not put member variables at the top of the class declaration, because they are not interesting as API.

> 2; function gobj() needlessly duplicated (only the a const version with GNode*
> return is needed) and public (should be protected)

No, these should be public, and fully useful. They are in case we forget to wrap a C function. Without these, people couldn't call extra C functions with the C++ instance. 

I did make gobj() inline, as it is in other glibmm classes, and moved them to the top so they will be used inline by the other templated functions.


Please remember to patch the ChangeLog in future. For instance:

2008-08-06  Murray Cumming  <murrayc@murrayc.com>

	* glib/src/nodetree.hg: Make gobject_ and data_ private, 
	to remove them from protected API. Patch from Szilárd Pfeiffer.
	Bug #546485.
	Make gobj() inline.

(In reply to comment #2)
> > 2; function gobj() needlessly duplicated (only the a const version with GNode*
> > return is needed) and public (should be protected)
> 
> No, these should be public, and fully useful. They are in case we forget to
> wrap a C function. Without these, people couldn't call extra C functions with
> the C++ instance. 

I see and you are right, however the two versions of gobj() (const and non-const) cause compilation problems in case of other const member functions. For example:

child_count() member function calls gobj() (the const version is called because child_count() is also const) but it returns with a const GNode*, which is not so good for the wrapped g_node_child_count() as input parameter.

Maybe another function (like gobj() in attachment 115941 [details] [review], but maybe with other name) should be used in case of member functions (it could be private). I am really interested in your opinion.

> 
> I did make gobj() inline, as it is in other glibmm classes, and moved them to

It is a good idea, a forget to do it.

> the top so they will be used inline by the other templated functions.
> 

(In reply to comment #3)
> I see and you are right, however the two versions of gobj() (const and
> non-const) cause compilation problems in case of other const member functions.
> For example:
> 
> child_count() member function calls gobj() (the const version is called because
> child_count() is also const) but it returns with a const GNode*, which is not
> so good for the wrapped g_node_child_count() as input parameter.

C functions generally don't use const, because C doesn't support const properly. So you should use const_cast<>, as we do throughout glibmm and gtkmm.

(In reply to comment #4)
> (In reply to comment #3)
> > I see and you are right, however the two versions of gobj() (const and
> > non-const) cause compilation problems in case of other const member functions.
> > For example:
> > 
> > child_count() member function calls gobj() (the const version is called because
> > child_count() is also const) but it returns with a const GNode*, which is not
> > so good for the wrapped g_node_child_count() as input parameter.
> 
> C functions generally don't use const, because C doesn't support const
> properly. So you should use const_cast<>, as we do throughout glibmm and gtkmm.
> 

Of course I should do. The question is that which one is preferred do it in each const function directly or write a simple function (something like gobj()) and use use it to avoid the const_cast "everywhere".

I think it's OK as it is, though I think we do have something like gobj_unconst() in some strange places in gtkmm.

(In reply to comment #6)
> I think it's OK as it is, though I think we do have something like
> gobj_unconst() in some strange places in gtkmm.
> 

It is OK apart from the fact that you cannot compile your source if it uses any of the following functions:

child_position
depth
node_count
child_count
is_ancestor
get_max_height

I've made a patch with the usage of the suggested gobj_unconst function.

Created attachment 116640 [details] [review]
patch mentioned in comment 7

ChangeLog:

2008-07-29  Szilárd Pfeiffer  <szilard.pfeiffer@gmail.com>

    * glib/src/nodetree.hg: Implement function gobj_unconst() and use it
    in the necessary const member functions instead of gobj() to fix the
    compilation.
    Bug #546485.

Thanks. I have added uses of const_cast<> to those const functions. As I said above, I prefer not to add the gobj_unconst() function.

I don't see how this could have worked anyway without a const_cast<> in here:

  inline GNode* gobj_unconst() const
  {
    return gobject_;
  }


Please remember to open closed bugs if you are adding an extra patch to them. There's a risk they could be forgotten otherwise.

(In reply to comment #9)
> Thanks. I have added uses of const_cast<> to those const functions. As I said
> above, I prefer not to add the gobj_unconst() function.

My solutions depends on your opinion at comment 6 ("... I think we do have something like obj_unconst() in some strange places in gtkmm.").

> 
> I don't see how this could have worked anyway without a const_cast<> in here:
> 
>   inline GNode* gobj_unconst() const
>   {
>     return gobject_;
>   }

In NodeTree the type of gobject_ is GNode*, it's value is a non-const (always created by g_node_new). The gobj() functions (depends on their type) return with a const/non-const version of gobject_, so you cannot use them in case of a const function (comment 3), except you use const_cast<> (as you did) which casts a const version of a surely non-const GNode*, which is not so nice IMHO. 

 
> 
> Please remember to open closed bugs if you are adding an extra patch to them.
> There's a risk they could be forgotten otherwise.
> 
I'll try not to forget.