Re: [Patch] PR11504: -Wcast-qual and general constness issues withstl_tree.h

[Gawain Bolton <gbolton at free dot fr>]

Gabriel Dos Reis wrote:

> 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.
Let's put this in perspective.  All C-style casts were replaced, in some 
cases with reinterpret_cast where required.  C-style casts are even 
worse than reinterpret_cast because they can also cast away constness 
where reinterpret_cast cannot.  So the patch makes things better, not 
worse, with respect to casting.

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

> | +    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;
>       }
Personally I don't like this because it's less obvious what's happening 
and harder for people to read/understand/change/debug the code.

Anyway I tried using this template member function anyway, to see what 
would happen to compile times and generated code size but the compiler 
gave me the following error message:

/home/gawain/C++/gcc-dev/install/include/c++/3.4/bits/stl_tree.h: In static
  member function `static _Np std::_Rb_tree_node_base::_S_minimum(_Np) 
[with
  _Np = std::_Rb_tree_node<std::pair<const Fred, unsigned int> >*]':
/home/gawain/C++/gcc-dev/install/include/c++/3.4/bits/stl_tree.h:491:   
instantiated from `static std::_Rb_tree_node<_Val>* std::_Rb_tree<_Key, 
_Val, _KeyOfValue, _Compare, 
_Alloc>::_S_minimum(std::_Rb_tree_node<_Val>*) [with _Key = Fred, _Val = 
std::pair<const Fred, unsigned int>, _KeyOfValue = 
std::_Select1st<std::pair<const Fred, unsigned int> >, _Compare = 
std::less<Fred>, _Alloc = std::allocator<std::pair<const Fred, unsigned 
int> >]'
/home/gawain/C++/gcc-dev/install/include/c++/3.4/bits/stl_tree.h:764:   
instantiated from `std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, 
_Alloc>& std::_Rb_tree<_Key, _Val, _KeyOfValue, _Compare, 
_Alloc>::operator=(const std::_Rb_tree<_Key, _Val, _KeyOfValue, 
_Compare, _Alloc>&) [with _Key = Fred, _Val = std::pair<const Fred, 
unsigned int>, _KeyOfValue = std::_Select1st<std::pair<const Fred, 
unsigned int> >, _Compare = std::less<Fred>, _Alloc = 
std::allocator<std::pair<const Fred, unsigned int> >]'
/home/gawain/C++/gcc-dev/install/include/c++/3.4/bits/stl_multimap.h:228:   
instantiated from `std::multimap<_Key, _Tp, _Compare, _Alloc>& 
std::multimap<_Key, _Tp, _Compare, _Alloc>::operator=(const 
std::multimap<_Key, _Tp, _Compare, _Alloc>&) [with _Key = Fred, _Tp = 
unsigned int, _Compare = std::less<Fred>, _Alloc = 
std::allocator<std::pair<const Fred, unsigned int> >]'
cnu_multi_map_test.cc:587:   instantiated from here
/home/gawain/C++/gcc-dev/install/include/c++/3.4/bits/stl_tree.h:111: 
error: invalid
  conversion from `std::_Rb_tree_node_base*' to `
  std::_Rb_tree_node<std::pair<const Fred, unsigned int> >*'

> | +    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
I agree.  However, this is not possible because the member in question 
is in the parent _Rb_tree_base_iterator class.

I am working on another patch which removes the _Rb_tree_base_iterator 
class altogether and which would allow for this to be changed.

>         _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<>.
No, reinterpret_cast does not cast away constness!  Only const_cast can 
cast away constness.

There's something I don't understand here.  You say reinterpret_cast is 
a "red alert" but don't seem to mind casting away constness.  All 
casting is ugly since it assumes you "know what you're doing" but to me 
the most dishonest cast is const_cast not reinterpret_cast.

The reason I think const_casts are the most dishonest is that you have 
"lied" to the compiler about a variable be const.  I see the other types 
of casts as telling the compiler something like "trust me, I know what 
I'm doing, you can't see that this variable is also this type but it is".

> 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.
Only reinterpret_cast worked in some cases.  Once again, remember that 
these replace the even more liberal C-style casts.  These 
reinterpret_casts are not "new" casts.

Cheers,


Gawain

--
Gawain Bolton
Coignieres, France
PGP Info: Key server: http://wwwkeys.pgp.net
         Key id: 6EBEDEA6
         Fingerprint: 65C0 0030 21D1 7A01 546A  E7DB D60F 47E0 6EBE DEA6