Re: [PATCH] __debug::list use C++11 direct initialization

[Jonathan Wakely <jwakely at redhat dot com>]

On 09/10/18 07:11 +0200, François Dumont wrote:
> Here is the communication for my yesterday's patch which I thought svn 
> had failed to commit (I had to interrupt it).
>
> Similarly to what I've done for associative containers here is a 
> cleanup of the std::__debug::list implementation leveraging more on 
> C++11 direct initialization.
>
> I also made sure we use consistent comparison between 
> iterator/const_iterator in erase and emplace methods.
>
> 2018-10-08  François Dumont <fdumont@gcc.gnu.org>
>
>     * include/debug/list (list<>::cbegin()): Use C++11 direct
>     initialization.
>     (list<>::cend()): Likewise.
>     (list<>::emplace<>(const_iterator, _Args&&...)): Likewise.
>     (list<>::insert(const_iterator, initializer_list<>)): Likewise.
>     (list<>::insert(const_iterator, size_type, const _Tp&)): Likewise.
>     (list<>::erase(const_iterator, const_iterator)): Ensure consistent
>     iterator comparisons.
>     (list<>::splice(const_iterator, list&&, const_iterator,
>     const_iterator)): Likewise.
>
> Tested under Linux x86_64 Debug mode and committed.
>
> François

> diff --git a/libstdc++-v3/include/debug/list b/libstdc++-v3/include/debug/list
> index 8add1d596e0..879e1177497 100644
> --- a/libstdc++-v3/include/debug/list
> +++ b/libstdc++-v3/include/debug/list
> @@ -244,11 +244,11 @@ namespace __debug
> #if __cplusplus >= 201103L
>       const_iterator
>       cbegin() const noexcept
> -      { return const_iterator(_Base::begin(), this); }
> +      { return { _Base::begin(), this }; }
>
>       const_iterator
>       cend() const noexcept
> -      { return const_iterator(_Base::end(), this); }
> +      { return { _Base::end(), this }; }
>
>       const_reverse_iterator
>       crbegin() const noexcept
> @@ -405,8 +405,8 @@ namespace __debug
> 	emplace(const_iterator __position, _Args&&... __args)
> 	{
> 	  __glibcxx_check_insert(__position);
> -	  return iterator(_Base::emplace(__position.base(),
> -					std::forward<_Args>(__args)...), this);
> +	  return  { _Base::emplace(__position.base(),
> +				   std::forward<_Args>(__args)...), this };
> 	}
> #endif
>
> @@ -430,7 +430,7 @@ namespace __debug
>       insert(const_iterator __p, initializer_list<value_type> __l)
>       {
> 	__glibcxx_check_insert(__p);
> -	return iterator(_Base::insert(__p.base(), __l), this);
> +	return { _Base::insert(__p.base(), __l), this };
>       }
> #endif
>
> @@ -439,7 +439,7 @@ namespace __debug
>       insert(const_iterator __position, size_type __n, const _Tp& __x)
>       {
> 	__glibcxx_check_insert(__position);
> -	return iterator(_Base::insert(__position.base(), __n, __x), this);
> +	return { _Base::insert(__position.base(), __n, __x), this };
>       }
> #else
>       void
> @@ -465,7 +465,7 @@ namespace __debug
> 		_Base::insert(__position.base(),
> 			      __gnu_debug::__unsafe(__first),
> 			      __gnu_debug::__unsafe(__last)),
> -		  this
> +		this
> 	      };
> 	  else
> 	    return { _Base::insert(__position.base(), __first, __last), this };
> @@ -521,15 +521,17 @@ namespace __debug
> 	// _GLIBCXX_RESOLVE_LIB_DEFECTS
> 	// 151. can't currently clear() empty container
> 	__glibcxx_check_erase_range(__first, __last);
> -	for (_Base_const_iterator __victim = __first.base();
> +	for (__decltype(__first.base()) __victim = __first.base();

This change causes the regression.

I think the problem is that the decltype is a reference, so every time
the loop does ++__victim it changes the iterator stored in __first.