Closed tchaikov closed 2 months ago
a reproducer can be found at https://godbolt.org/z/K5nsj5azT
hi @jzmaddock , could you help review this change? my colleague @nyh believes that an "inf" should be returned instead, as it was the behaviour in boost 1.78. what do you think?
It looks like it's fine: but your parenthesis are in the wrong place around make_policy, should be (I hope):
*result = boost::math::float_next(*result, boost::math::policies::make_policy(boost::math::policies::overflow_error<boost::math::policies::ignore_error>(), boost::math::policies::domain_error<boost::math::policies::ignore_error>()));
Never mind, I see I can edit, have changed thing round...
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 94.1%. Comparing base (
fe3054f
) to head (ea78649
).
Failures should be corrected by https://github.com/boostorg/math/pull/1130 once it runs...
Never mind, I see I can edit, have changed thing round...
thank you! squashed and repushed.
hi @jzmaddock , could you help review this change? my colleague @nyh believes that an "inf" should be returned instead, as it was the behaviour in boost 1.78. what do you think?
I stand by my belief that when converting an integer to floating-point, truncation should result in +Inf (or -Inf) and not NaN. Returning Inf in the case of truncation, and not NaN, is traditional, for example:
>>> print(1000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000.0)
inf
but even more importantly, this also this was the behaviour in older Boost before this bug happened, and application code may rely on this truncation behavior for its own user-visible truncation (as for example happened in the ScyllaDB project).
So although after this patch the conversion no longer crashes (good), it's still wrong in my view.
My bad, you're quite correct, see https://github.com/boostorg/math/issues/1132.
My bad, you're quite correct, see boostorg/math#1132.
Thanks. I'm not familiar with Boost's unit tests, but if you have them, I recommend that you add regression tests for convert_to() whose results are expected to be both +inf and -inf, because it's possible the details of what broke (float_nex()t? I didn't understand the details) would be different in the two cases. Although float_next(+inf) might perhaps be defined, what is float_next(-inf) supposed to return?
UPDATE: I read the convert_to code again, and it seems it works on the positive number, and always adds the sign at the end - so it will always be +Inf, not -Inf, at this stage. So I think the code is indeed fine, and you just need float_next(+Inf) to return +Inf. Or, perhaps not call this function in the case of Inf?
this change is a follow-up of d51f2e9dbb. it intends to address the exception thrown in a noexcept functon.
a minimal reproducer looks like
since boost 1.79, the code above terminates like
because
float_next_imp()
throwsboost::wrapexcept<std::domain_error>
if the number is NAN of INF. andeval_convert_to()
is marked asnoexcept(boost::multiprecision::detail::is_arithmetic<R>::value && std::numeric_limits<R>::has_infinity)
, but onlyoverflow_error
is ignored in the policy passed tofloat_next()
.so, in this change,
std::domain_error
is ignored as well, so thatnum.convert_to<float>()
returns a NaN in this case.Refs #553