Re: Relocation (= move+destroy)

[Jonathan Wakely <jwakely at redhat dot com>]

CCing gcc-patches

On 19/10/18 07:33 +0200, Marc Glisse wrote:
> On Thu, 18 Oct 2018, Marc Glisse wrote:
>
> > Uh, why didn't I notice that the function __relocate is unused? I 
> > guess I'll resend the same patch without __relocate once retesting 
> > has finished :-( Sorry for all the corrections, I guess I didn't 
> > check my patch carefully enough before sending it the first time.
>
> 2018-10-19  Marc Glisse  <marc.glisse@inria.fr>
>
>        PR libstdc++/87106
>        * include/bits/alloc_traits.h (_S_construct, _S_destroy, construct,
>        destroy): Add noexcept specification.
>        * include/bits/allocator.h (construct, destroy): Likewise.
>        * include/ext/alloc_traits.h (construct, destroy): Likewise.
>        * include/ext/malloc_allocator.h (construct, destroy): Likewise.
>        * include/ext/new_allocator.h (construct, destroy): Likewise.
> 	* include/bits/stl_uninitialized.h (__relocate_a, __relocate_a_1):
> 	New functions.
>        (__is_trivially_relocatable): New class.
>        * include/bits/stl_vector.h (__use_relocate): New static member.
>        * include/bits/vector.tcc (reserve, _M_realloc_insert,
>        _M_default_append): Use __relocate_a.
>        (reserve, _M_assign_aux, _M_realloc_insert, _M_fill_insert,
>        _M_default_append, _M_range_insert): Move _GLIBCXX_ASAN_ANNOTATE_REINIT
>        after _Destroy.
>        * testsuite/23_containers/vector/modifiers/push_back/49836.cc:
>        Replace CopyConsOnlyType with DelAnyAssign.
>
> --
> Marc Glisse

The tricky stuff in <bits/stl_vector.h> all looks right, I only have
some comments on the __relocate_a functions ...


> Index: libstdc++-v3/include/bits/stl_uninitialized.h
> ===================================================================
> --- libstdc++-v3/include/bits/stl_uninitialized.h	(revision 265289)
> +++ libstdc++-v3/include/bits/stl_uninitialized.h	(working copy)
> @@ -872,14 +872,75 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     uninitialized_move_n(_InputIterator __first, _Size __count,
> 			 _ForwardIterator __result)
>     {
>       auto __res = std::__uninitialized_copy_n_pair
> 	(_GLIBCXX_MAKE_MOVE_ITERATOR(__first),
> 	 __count, __result);
>       return {__res.first.base(), __res.second};
>     }
> #endif
>
> +#if __cplusplus >= 201402L

What depends on C++14 here? Just enable_if_t? Because we have
__enable_if_t for use in C++11.

Both GCC and Clang will allow constexpr-if and static_assert with no
message in C++11.

> +  template<typename _Tp, typename _Up, typename _Allocator>
> +    inline void
> +    __relocate_a(_Tp* __dest, _Up* __orig, _Allocator& __alloc)

I find it a little surprising that this overload for single objects
using the memmove argument ordering (dest, source) but the range overload
below uses the STL ordering (source_begin, source_end, dest).

But I wouldn't be surprised if we're already doing that somewhere that
I've forgotten about.

WOuld it make sense to either rename this overload, or to use
consistent argument ordering for the two __relocate_a overloads?

> +    noexcept(noexcept(__gnu_cxx::__alloc_traits<_Allocator>::construct(__alloc,

Since this is C++14 (or maybe C++11) you could just use
std::allocator_traits directly. __gnu_cxx::__alloc_traits is to
provide equivalent functionality in C++98 code.

> +			 __dest, std::move(*__orig)))
> +	     && noexcept(__gnu_cxx::__alloc_traits<_Allocator>::destroy(
> +			    __alloc, std::__addressof(*__orig))))
> +    {
> +      typedef __gnu_cxx::__alloc_traits<_Allocator> __traits;
> +      __traits::construct(__alloc, __dest, std::move(*__orig));
> +      __traits::destroy(__alloc, std::__addressof(*__orig));
> +    }
> +
> +  template<typename _Tp>
> +    struct __is_trivially_relocatable
> +    : is_trivial<_Tp> { };

It might be worth adding a comment that this type might be specialized
in future, so that I don't forget and simplify it to an alias template
later :-)

> +  template <typename _Tp, typename _Up>
> +    inline std::enable_if_t<std::__is_trivially_relocatable<_Tp>::value, _Tp*>
> +    __relocate_a_1(_Tp* __first, _Tp* __last,
> +		   _Tp* __result, allocator<_Up>& __alloc)
> +    {
> +      ptrdiff_t __count = __last - __first;
> +      __builtin_memmove(__result, __first, __count * sizeof(_Tp));
> +      return __result + __count;
> +    }
> +
> +  template <typename _InputIterator, typename _ForwardIterator,
> +	    typename _Allocator>
> +    inline _ForwardIterator
> +    __relocate_a_1(_InputIterator __first, _InputIterator __last,
> +		   _ForwardIterator __result, _Allocator& __alloc)
> +    {
> +      typedef typename iterator_traits<_InputIterator>::value_type
> +	_ValueType;
> +      typedef typename iterator_traits<_ForwardIterator>::value_type
> +	_ValueType2;
> +      static_assert(std::is_same<_ValueType, _ValueType2>::value);
> +      static_assert(noexcept(std::__relocate_a(std::addressof(*__result),
> +					       std::addressof(*__first),
> +					       __alloc)));
> +      _ForwardIterator __cur = __result;
> +      for (; __first != __last; ++__first, (void)++__cur)
> +	std::__relocate_a(std::__addressof(*__cur),
> +			  std::__addressof(*__first), __alloc);
> +      return __cur;
> +    }
> +
> +  template <typename _InputIterator, typename _ForwardIterator,
> +	    typename _Allocator>
> +    inline _ForwardIterator
> +    __relocate_a(_InputIterator __first, _InputIterator __last,
> +		 _ForwardIterator __result, _Allocator& __alloc)
> +    {
> +      return __relocate_a_1(std::__niter_base(__first),
> +			    std::__niter_base(__last),
> +			    std::__niter_base(__result), __alloc);
> +    }
> +#endif
> +
> _GLIBCXX_END_NAMESPACE_VERSION
> } // namespace