[libc++] Add `__truncating_cast` for safely casting float types to integers

[ldionne]

Summary

This is needed anytime we need to clamp an arbitrary floating point value to an integer type.

[zoecarver]

Seems like a very useful function. __max_representable_int_for_float also seems useful. Should this work in C++03? If so there are a few changes that need to be made. It would also be great if this could be a constexpr (but, obviously, not necessary).

include/math.h

1556 ↗

(On Diff #217513)

Seems odd this is the only thing in this file inside the standard namespace. Are we moving towards writing std::__helper instead of __libcpp_helper? It seems like the other helper functions in this file use the __libcpp prefix and aren't in the standard namespace.

1558 ↗

(On Diff #217513)

Nit: maybe qualify all the uses of numeric_limits and similar?

1572 ↗

(On Diff #217513)

What is the enum providing for you? Couldn't this just be static const int _Bits = ...?

1573 ↗

(On Diff #217513)

What's the reasoning behind shifting something forward and back? Shouldn't this always negate the other operation?

1579 ↗

(On Diff #217513)

This will not work before C++11.

1582 ↗

(On Diff #217513)

Maybe change INFINITY to std::numeric_limits< _RealT >::infinity()

test/libcxx/numerics/truncating_cast.pass.cpp

10 ↗

(On Diff #217513)

Is this supposed to work in C++03? If so, update this test and __truncating_cast. Otherwise, add an #if and a // UNSUPPORTED: C++98, C++03

25 ↗

(On Diff #217513)

Maybe test with more than just double. float, long double, others?

28 ↗

(On Diff #217513)

C++03 will not like this :P

[scanon]

I would tend to write this function in the following form:

// set up lower bound and upper bound
if (r > upperBound) r = upperBound;
if (!(r >= lowerBound)) r = lowerBound; // NaN is mapped to l.b.
return static_cast<IntType>(r);

I prefer to avoid the explicit trunc call, since that's the defined behavior of the static_cast once the value is in-range, anyway.

include/math.h

1573 ↗

(On Diff #217513)

This function doesn't quite do what it says on the tin; it considers the number of significand bits used for the floating-point type, but not the exponent range. This doesn't matter for double, because double's exponent range is much, much larger than any integer type, but it does matter for types like float16 (largest representable value is 65504)--when it's added as a standard floating-point type at some future point, this will introduce subtle bugs.

You should be able to work around this by converting value to _FloatT, taking the minimum of the result and numeric_limits::max, and converting back.

This also assumes that _FloatT has radix == 2, which I do not believe is actually implied by is_floating_point == true. Please add a static assert for that so that future decimal types don't use this template.

1582 ↗

(On Diff #217513)

Why isn't this just __trunc_r > _MaxVal?

1584 ↗

(On Diff #217513)

This has a subtle assumption that _IntT is two's-complement and _FloatT has radix=2, so that the implicit conversion that occurs in the comparison is exact. The radix should be a static assert; does libc++ care about non-two's-complement at all?

Just from a clarity perspective, I would personally make the conversion explicit.

1586 ↗

(On Diff #217513)

If I'm reading right, NaNs will fall through the above two comparisons and invoke UB on the static_cast below. I suspect that's not the desired behavior. What is the intended result for NaN?

[scanon]

test/libcxx/numerics/truncating_cast.pass.cpp

36 ↗

(On Diff #217513)

Probably should test nextafter(static_cast<double>(Lim::max()), INFINITY) here instead.

[EricWF]

include/math.h

1556 ↗

(On Diff #217513)

We shouldn't put things in the global namespace generally. It's fine that this is the only thing in namespace std is this file.

1558 ↗

(On Diff #217513)

No need to qualify types. The lookup will be unambiguous.

1572 ↗

(On Diff #217513)

It subtly enforces that the argument is a compile time constant without introducing a global variable declaration.

1579 ↗

(On Diff #217513)

Using statements? Yes it will, but only because we require Clang in C++03 now. And Clang allows using statements as an extension.

1582 ↗

(On Diff #217513)

Consider long long and double. MaxVal - numeric_limits<long long>::max() == 1024, and we want values between MaxVal and ::max() to round down. So instead we essentially check for __r >= numeric_limits<long long>::max() + 1. This approach seems more accurate.

1584 ↗

(On Diff #217513)

I'll static assert the radix, but I think it's safe to assume twos compliment. According to p0907, none of MSVC, GCC, or Clang support other representations [1].

How would you make this explicit?

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r0.html

1586 ↗

(On Diff #217513)

I didn't want to treat NaN as a valid input, so I want to allow UBSAN to catch it rather than to provide a valid output.

test/libcxx/numerics/truncating_cast.pass.cpp

10 ↗

(On Diff #217513)

The test compiles and passes with C++03.

[EricWF]

Address review comments. I think this is good to go.

[scanon]

include/math.h

1582 ↗

(On Diff #217513)

Consider long long and double. MaxVal - numeric_limits<long long>::max() == 1024, and we want values between MaxVal and ::max() to round down. So instead we essentially check for __r >= numeric_limits<long long>::max() + 1

Yes, but there are no values between MaxVal and ::max() in the floating-point format; if there were, they would be MaxVal instead. So you can ditch the nextafter and just use > static_cast<_FloatT>(MaxVal).

[scanon]

include/math.h

1586 ↗

(On Diff #217513)

Please document this clearly; otherwise someone will assume that this is a UB-free conversion and use it for that purpose.

[EricWF]

include/math.h

1573 ↗

(On Diff #217513)

Very good point. I've added the static assertions and limited the function to float, double, and long double so the fp16 case won't bite us anytime soon.

[zoecarver]

include/math.h

1582 ↗

(On Diff #217513)

I thought the same thing, but that isn't necessarily true. Eric showed me this link https://godbolt.org/z/AjBHYqv which does a good job showing what happens when trying to compare an integer value and float value. See the precision loss:

warning: implicit conversion from 'long long' to 'double' changes value from 9223372036854775807 to 9223372036854775808

[scanon]

Eric showed me this link https://godbolt.org/z/AjBHYqv

Dead link.

[zoecarver]

Dead link.

Here: https://godbolt.org/z/AjBHYq

[scanon]

In D66836#1649846, @zoecarver wrote:

Dead link.

Here: https://godbolt.org/z/AjBHYq

Yes, conversion of numeric_limits<long long>::max to double rounds to a value out of range for long long. That's not what I'm talking about. Very specifically, in this line:

if (__r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY))

_MaxVal, by construction, is representable both as _RealT and as _IntT, so the static_cast does not change the value (so the rounding demonstrated in your godbolt link doesn't create a bug). a >= nextafter(b, INFINITY) is equivalent to a > b for any finite floating-point a and b. So this condition can simply be if (__r > static_cast<_RealT>(_MaxVal)).

[zoecarver]

Yes, that sounds right. I can't think of any reason that the condition couldn't be if (__r > static_cast<floatT>(numeric_limits<intT>::max())). The information lost from shifting the value around is never more than the information lost from static_casting the value (as far as I have been able to reason and test).

[majnemer]

test/libcxx/numerics/truncating_cast.pass.cpp

12 ↗

(On Diff #217513)

closes -> closest

[EricWF]

Address review comments.

• Document that NaN isn't a supported input.
• Fix spelling mistake.

[ldionne]

Taking over because Eric is on vacation. I think everything has been addressed at this point.

@scanon do you see anything else that needs to change?

[scanon]

I believe that the code can still be simplified somewhat, but that it's correct as-is for float, double, and long double. I'll take an AI to follow-up on future improvements, and let's get this in.

libcxx/trunk/include/math.h

 inline _LIBCPP_INLINE_VISIBILITY float       trunc(float __lcpp_x) _NOEXCEPT       {return ::truncf(__lcpp_x);}
 inline _LIBCPP_INLINE_VISIBILITY long double trunc(long double __lcpp_x) _NOEXCEPT {return ::truncl(__lcpp_x);}
 
 template <class _A1>
 inline _LIBCPP_INLINE_VISIBILITY
 typename std::enable_if<std::is_integral<_A1>::value, double>::type
 trunc(_A1 __lcpp_x) _NOEXCEPT {return ::trunc((double)__lcpp_x);}
 
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+template <class _IntT, class _FloatT,
+    bool _FloatBigger = (numeric_limits<_FloatT>::digits > numeric_limits<_IntT>::digits),
+    int _Bits = (numeric_limits<_IntT>::digits - numeric_limits<_FloatT>::digits)>
+_LIBCPP_INLINE_VISIBILITY
+_LIBCPP_CONSTEXPR _IntT __max_representable_int_for_float() _NOEXCEPT {
+  static_assert(is_floating_point<_FloatT>::value, "must be a floating point type");
+  static_assert(is_integral<_IntT>::value, "must be an integral type");
+  static_assert(numeric_limits<_FloatT>::radix == 2, "FloatT has incorrect radix");
+  static_assert(_IsSame<_FloatT, float>::value || _IsSame<_FloatT, double>::value
+               || _IsSame<_FloatT,long double>::value, "unsupported floating point type");
+  return _FloatBigger ? numeric_limits<_IntT>::max() :  (numeric_limits<_IntT>::max() >> _Bits << _Bits);
+}
+
+// Convert a floating point number to the specified integral type after
+// clamping to the integral types representable range.
+//
+// The behavior is undefined if `__r` is NaN.
+template <class _IntT, class _RealT>
+_LIBCPP_INLINE_VISIBILITY
+_IntT __clamp_to_integral(_RealT __r) _NOEXCEPT {
+  using _Lim = std::numeric_limits<_IntT>;
+  const _IntT _MaxVal = std::__max_representable_int_for_float<_IntT, _RealT>();
+  if (__r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) {
+    return _Lim::max();
+  } else if (__r <= _Lim::lowest()) {
+    return _Lim::min();
+  }
+  return static_cast<_IntT>(__r);
+}
+
+_LIBCPP_END_NAMESPACE_STD
+
 } // extern "C++"
 
 #endif // __cplusplus
 
 #else // _LIBCPP_MATH_H
 
 // This include lives outside the header guard in order to support an MSVC
 // extension which allows users to do:

libcxx/trunk/test/libcxx/numerics/clamp_to_integral.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// __clamp_to_integral<IntT>(RealT)
+
+// Test the conversion function that truncates floating point types to the
+// closest representable value for the specified integer type, or
+// numeric_limits<IntT>::max()/min() if the value isn't representable.
+
+#include <limits>
+#include <cassert>
+#include <cmath>
+
+template <class IntT>
+void test() {
+  typedef std::numeric_limits<IntT> Lim;
+  const bool MaxIsRepresentable = sizeof(IntT) < 8;
+  const bool IsSigned = std::is_signed<IntT>::value;
+  struct TestCase {
+    double Input;
+    IntT Expect;
+    bool IsRepresentable;
+  } TestCases[] = {
+      {0, 0, true},
+      {1, 1, true},
+      {IsSigned ? static_cast<IntT>(-1) : 0,
+       IsSigned ? static_cast<IntT>(-1) : 0, true},
+      {Lim::lowest(), Lim::lowest(), true},
+      {static_cast<double>(Lim::max()), Lim::max(), MaxIsRepresentable},
+      {static_cast<double>(Lim::max()) + 1, Lim::max(), false},
+      {static_cast<double>(Lim::max()) + 1024, Lim::max(), false},
+      {nextafter(static_cast<double>(Lim::max()), INFINITY), Lim::max(), false},
+  };
+  for (TestCase TC : TestCases) {
+    auto res = std::__clamp_to_integral<IntT>(TC.Input);
+    assert(res == TC.Expect);
+    if (TC.IsRepresentable) {
+      auto other = static_cast<IntT>(std::trunc(TC.Input));
+      assert(res == other);
+    } else
+      assert(res == Lim::min() || res == Lim::max());
+  }
+}
+
+template <class IntT>
+void test_float() {
+  typedef std::numeric_limits<IntT> Lim;
+  const bool MaxIsRepresentable = sizeof(IntT) < 4;
+  ((void)MaxIsRepresentable);
+  const bool IsSigned = std::is_signed<IntT>::value;
+  struct TestCase {
+    float Input;
+    IntT Expect;
+    bool IsRepresentable;
+  } TestCases[] = {
+      {0, 0, true},
+      {1, 1, true},
+      {IsSigned ? static_cast<IntT>(-1) : 0,
+       IsSigned ? static_cast<IntT>(-1) : 0, true},
+      {Lim::lowest(), Lim::lowest(), true},
+      {static_cast<float>(Lim::max()), Lim::max(), MaxIsRepresentable },
+       {nextafter(static_cast<float>(Lim::max()), INFINITY), Lim::max(), false},
+  };
+  for (TestCase TC : TestCases) {
+    auto res = std::__clamp_to_integral<IntT>(TC.Input);
+    assert(res == TC.Expect);
+    if (TC.IsRepresentable) {
+      auto other = static_cast<IntT>(std::trunc(TC.Input));
+      assert(res == other);
+    } else
+      assert(res == Lim::min() || res == Lim::max());
+  }
+}
+
+int main() {
+  test<short>();
+  test<unsigned short>();
+  test<int>();
+  test<unsigned>();
+  test<long long>();
+  test<unsigned long long>();
+  test_float<short>();
+  test_float<int>();
+  test_float<long long>();
+}