Opened 5 years ago

Last modified 5 years ago

#13487 new Bugs

Error detecting is_nothrow_move_constructible & is_nothrow_move_assignable

Reported by: Andrey Glebov <andrey458641387@…> Owned by: Ion Gaztañaga
Milestone: To Be Determined Component: move
Version: Boost Development Trunk Severity: Problem
Keywords: move noexcept nothrow Cc:

Description

The current type trait implementation of Boost.Move doesn't detect nothrow move construction or assignment.

While boost/move/detail/type_traits.hpp defines many macros such as BOOST_MOVE_HAS_NOTHROW_COPY, it never defines BOOST_MOVE_HAS_NOTHROW_MOVE or BOOST_MOVE_HAS_NOTHROW_MOVE_ASSIGN for any compiler. As a result, boost::move:detail::is_nothrow_move_constructible and is_nothrow_move_assignable fallback to boost::move_detail::is_pod<T>::value which is incorrect for any non-POD type.

In my particular instance the result is that boost::circular_buffer chooses copy instead of move in boost::move_if_noexcept() for std::weak_ptr (which is nothrow move constructible and assignable), which in turn results in >10x worse performance of functions such as boost::circular_buffer::erase() in some conditions. It is possible to work around this by manually specializing boost::has_nothrow_move<std::weak_ptr>, but this is not a practical workaround. Another workaround would be to define the trait macros via compiler switches but this is clearly not the intended behavior as all other macros are defined in the header, and it might not be practical as some helper traits may need to be defined too (see libstdc++ implementation below).

I propose fixing this issue by defining BOOST_MOVE_HAS_NOTHROW_MOVE and BOOST_MOVE_HAS_NOTHROW_MOVE_ASSIGN for all compilers that support move semantics. libstdc++ seems to have the most correct and portable way of doing this - without using compiler intrinsics:

  template<typename _Tp, typename... _Args>
    struct __is_nt_constructible_impl
    : public integral_constant<bool, noexcept(_Tp(declval<_Args>()...))>
    { };

  template<typename _Tp, typename _Arg>
    struct __is_nt_constructible_impl<_Tp, _Arg>
    : public integral_constant<bool,
                               noexcept(static_cast<_Tp>(declval<_Arg>()))>
    { };


  template<typename _Tp, bool = __is_referenceable<_Tp>::value>
    struct __is_nothrow_move_constructible_impl;

  template<typename _Tp>
    struct __is_nothrow_move_constructible_impl<_Tp, false>
    : public false_type { };

  template<typename _Tp>
    struct __is_nothrow_move_constructible_impl<_Tp, true>
    : public is_nothrow_constructible<_Tp, _Tp&&>
    { };


  template<typename _Tp, typename _Up>
    struct __is_nt_assignable_impl
    : public integral_constant<bool, noexcept(declval<_Tp>() = declval<_Up>())>
    { };


  template<typename _Tp, bool = __is_referenceable<_Tp>::value>
    struct __is_nt_move_assignable_impl;

  template<typename _Tp>
    struct __is_nt_move_assignable_impl<_Tp, false>
    : public false_type { };

  template<typename _Tp>
    struct __is_nt_move_assignable_impl<_Tp, true>
    : public is_nothrow_assignable<_Tp&, _Tp&&>
    { };

Visual Studio 14.0's implementation uses the intrinsics __is_nothrow_constructible(T, Args...) and __is_nothrow_assignable(To, From) that are passed an rvalue-reference as the second argument to detect move operations. Sadly, I can't seem to find any documentation on these intrinsics so it might be better to stick to pure C++ for all compilers (or maybe i'm just not looking in the right places).

It might also be a good idea to replace the fallback to boost::move_detail::is_pod<T> with more specific and inclusive is_trivially_copy_constructible<T>, is_trivially_copy_assignable<T> type traits.

Change History (1)

comment:1 by Andrey Glebov <andrey458641387@…>, 5 years ago

Note on the libstdc++ implementation of is_nothrow_move_assignable: it actually also contains:

  template<typename _Tp, typename _Up>
    struct is_nothrow_assignable
    : public __and_<is_assignable<_Tp, _Up>,
                    __is_nt_assignable_impl<_Tp, _Up>>
    { };

where is_assignable<_Tp, _Up> is implemented in terms of the intrinsic __is_assignable(To, From). This shouldn't be an issue though since that intrinsic seems to be supplied by most compilers.

Note: See TracTickets for help on using tickets.