cpp_rational is not nothrow move constructible

[Taras Morozovsky <taras.morozovsky@…>]

boost::multiprecision::cpp_rational's move constructor is not noexcept. This does not allow programmers to make objects containing a cpp_rational data member nothrow move constructible. Maybe that's not a severe bug, but an inconvenience at least.

#include <iostream> #include <iomanip> #include <type_traits> #include <boost/multiprecision/cpp_int.hpp> int main() {

using namespace std; using namespace boost::multiprecision; cout << boolalpha; cout << is_nothrow_move_constructible<cpp_int>::value << endl; cout << is_nothrow_move_assignable<cpp_int>::value << endl; cout << is_nothrow_move_constructible<cpp_rational>::value << endl; cout << is_nothrow_move_assignable<cpp_rational>::value << endl;

}

Outputs: true true false true

I believe there is a problem in rational_adaptor (see <rational_adaptor.hpp>, line 60), since its move constructor is defined as follows:

rational_adaptor(rational_adaptor&& o) : m_value(o.m_value) {}

Here, o.m_value is treated like an lvalue despite it can be moved from. Thus new object's m_value is copy-constructed, not move-constructed. That does not allow cpp_rational's move constructor to be noexcept (it is noexcept only if its backend class has a noexcept move ctor), also it may lead to some minor loss of efficiency.

This problem can be fixed by rewriting mentioned rational_adaptor's move constructor this way:

rational_adaptor(rational_adaptor&& o)

BOOST_NOEXCEPT_IF(noexcept(rational_type(std::declval<rational_type>())))

: m_value(static_cast<rational_type&&>(o.m_value)) {}

[John Maddock]

Thanks for the catch, I need to do some more testing of the other types in the library but for now here's the patch for this this specific case: ​https://github.com/boostorg/multiprecision/commit/33630bc407b044a98cd8c3b0dce93ca22a01a002

[Taras Morozovsky <taras.morozovsky@…>]

Replying to johnmaddock:

Thanks for the catch, I need to do some more testing of the other types in the library but for now here's the patch for this this specific case

Thank you for your reply!

Please, keep in mind that arguments received by rvalue reference parameters are themselves lvalues. And so, in order to enable moving from them, they must be cast to rvalues once again. Without it, copy operations will be invoked instead, which may be dangerous since they may throw exceptions (std::bad_alloc in particular) when we promise that current operation will never throw.

That's why ...: m_value(o.m_value) {} at line 60 and ...: m_value(o) {} at line 61 should be turned into ...: m_value(static_cast<rational_type&&>(o.m_value)) {} and ...: m_value(static_cast<IntBackend&&>(o)) {} respectively.

Considering line 61, I believe this std::declval<integer_type>() must be std::declval<IntBackend>() instead, since a rational_type (m_value) is being constructed from an rvalue reference to IntBackend (o), not from a number<IntBackend> (which integer_type is).

And one more thing I noticed: the condition inside BOOST_NOEXCEPT_IF at line 62. Now, it contains an expression std::declval<rational_type>() = std::declval<rational_type>() inside the noexcept operator. I think, it should be std::declval<rational_type&>() = std::declval<rational_type>() instead (with an lvalue reference on the left-hand side), because m_value is itself an lvalue at line 64 where it is being assigned to.

Hope this will help someway!

[John Maddock]

Thanks, you're right of course, many more fixes in ​https://github.com/boostorg/multiprecision/commit/9927d49cb9d87c022c3ac49b3c8d84ce55ab7dc4.

Aside: I find it deeply frustrating that noexcept allows you to shoot yourself in the foot like this - the very least the compiler could do is issue a warning in the case that a noexcept function calls a non-noexcept one, otherwise I fear this is painfully easy to break!

Please reopen if you spot any other issues.