[Patch] PR11504: -Wcast-qual and general constness issues with stl_tree.h

[Gabriel Dos Reis]

Gawain Bolton <gbolton@free.fr> writes:

|    1. I do not like the lack of symmetry with the casts for const vs.
|       non-const functions.
|       Non-const versions often require a reinterpret_cast whereas const
|       versions can use static_cast.

Use of reinterpret_cast is a red alert.

As a general comment, it is preferable to use "diff -p" instead of
"diff -u".

| +    static _const_Base_ptr
| +    _S_minimum(_const_Base_ptr __x)
| +    {
| +      while (__x->_M_left != 0) __x = __x->_M_left;
| +      return __x;
| +    }
| +

Why repeat twice the same tokens, especially when the difference is
only in the type of the argument?

     template<class _Np>
       static _Np
       _S_minimum(_Np __x)
       {
          while (__x->_M_left != 0)
            __x = __x->_M_left;
          return __x;
       }

| +    static _const_Base_ptr
| +    _S_maximum(_const_Base_ptr __x)
| +    {
| +      while (__x->_M_right != 0) __x = __x->_M_right;
| +      return __x;
| +    }

Ditto.

|    template<typename _Val>
| @@ -149,13 +164,15 @@
|        const_iterator;
|        typedef _Rb_tree_iterator<_Val, _Ref, _Ptr> _Self;
|        typedef _Rb_tree_node<_Val>* _Link_type;
| +      typedef const _Rb_tree_node<_Val>* _const_Link_type;
|        
|        _Rb_tree_iterator() {}
| -      _Rb_tree_iterator(_Rb_tree_node_base* __x) { _M_node = __x; }
| +      _Rb_tree_iterator(_Link_type __x) { _M_node = __x; }

We should strive for member-initializer lists, where possible

         _Rb_tree_iterator(_Link_type __x) : _M_node(__x) { }


| +      _Rb_tree_iterator(_const_Link_type __x) { _M_node = const_cast<_Link_type>(__x); }

Ditto.

[...]

|        _Link_type& 
| -      _M_root() const { return (_Link_type&) this->_M_header._M_parent; }
| +      _M_root() { return reinterpret_cast<_Link_type&>(this->_M_header._M_parent); }
| +
| +      _const_Link_type
| +      _M_root() const { return static_cast<_const_Link_type>(this->_M_header._M_parent); }

I do not understand why we would need 'reinterpret_cast' here.
If the issue is to safely cast away const-ness, the use const_cast<>.
IF the issue is to safely to derived class, then static_cast<> will do
the job.  I do not see opportunity for reinterpret_cast<> here.

-- Gaby