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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 10 inline comments as done.
EricWF added inline comments.


================
Comment at: include/math.h:1556
 
+_LIBCPP_BEGIN_NAMESPACE_STD
+template <class _IntT, class _FloatT,
----------------
zoecarver wrote:
> 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. 
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.


================
Comment at: include/math.h:1558
+template <class _IntT, class _FloatT,
+    bool _FloatBigger = (numeric_limits<_FloatT>::digits > numeric_limits<_IntT>::digits)>
+struct __max_representable_int_for_float;
----------------
zoecarver wrote:
> Nit: maybe qualify all the uses of `numeric_limits` and similar?
No need to qualify types. The lookup will be unambiguous. 


================
Comment at: include/math.h:1572
+  static_assert(is_integral<_IntT>::value, "must be an integral type");
+  enum { _Bits = numeric_limits<_IntT>::digits - numeric_limits<_FloatT>::digits };
+  static const _IntT value = numeric_limits<_IntT>::max() >> _Bits << _Bits;
----------------
zoecarver wrote:
> What is the enum providing for you? Couldn't this just be `static const int _Bits  = ...`?
It subtly enforces that the argument is a compile time constant without introducing a global variable declaration.


================
Comment at: include/math.h:1579
+_IntT __truncating_cast(_RealT __r) _NOEXCEPT {
+  using _Lim = std::numeric_limits<_IntT>;
+  const _IntT _MaxVal = __max_representable_int_for_float<_IntT, _RealT>::value;
----------------
zoecarver wrote:
> This will not work before C++11.
Using statements? Yes it will, but only because we require Clang in C++03 now. And Clang allows using statements as an extension.


================
Comment at: include/math.h:1582
+  const _RealT __trunc_r = __builtin_trunc(__r);
+  if (__trunc_r >= ::nextafter(static_cast<_RealT>(_MaxVal), INFINITY)) {
+    return _Lim::max();
----------------
scanon wrote:
> zoecarver wrote:
> > Maybe change `INFINITY` to `std::numeric_limits< _RealT >::infinity()`
> Why isn't this just `__trunc_r > _MaxVal`?
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.


================
Comment at: include/math.h:1584
+    return _Lim::max();
+  } else if (__trunc_r <= _Lim::lowest()) {
+    return _Lim::min();
----------------
scanon wrote:
> 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.
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


================
Comment at: include/math.h:1586
+    return _Lim::min();
+  }
+  return static_cast<_IntT>(__trunc_r);
----------------
scanon wrote:
> 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?
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.


================
Comment at: test/libcxx/numerics/truncating_cast.pass.cpp:10
+// __truncating_cast<IntT>(RealT)
+
+// Test the conversion function that truncates floating point types to the
----------------
zoecarver wrote:
> 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`
The test compiles and passes with C++03.


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66836/new/

https://reviews.llvm.org/D66836