Re: Relocation (= move+destroy)

[Marc Glisse <marc dot glisse at inria dot fr>]

On Tue, 23 Oct 2018, Jonathan Wakely wrote:

> CCing gcc-patches

It seems to have disappeared somehow during the discussion, sorry.

> 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.

Probably it can be enabled in C++11 if you think that matters. I'll admit 
that I personally don't care at all about C++11, and the main motivation 
would be to enable a cleanup if we stop supporting C++03 (I am not very 
optimistic).

> > +  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?

The functions were not meant as overloads, it just happened that I arrived 
at the same name for both, but it would make perfect sense to give them 
different names. I started from __relocate(dest, source) for one element, 
and later added an allocator to it. The other one corresponds to 
__uninitialized_move_a, and naming it __uninitialized_relocate_a would be 
silly since "uninitialized" is included in the definition of relocate.

I think I'd rather rename than change the order. Do you have suggestions? 
__relocate_range_a?

> > + 
> > 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.

Thanks, I was wondering what it was for.

> > +			 __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 :-)

Ok.

--
Marc Glisse