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

[Steve Canon via Phabricator via libcxx-commits]

scanon added a comment.

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.



================
Comment at: include/math.h:1573
+  enum { _Bits = numeric_limits<_IntT>::digits - numeric_limits<_FloatT>::digits };
+  static const _IntT value = numeric_limits<_IntT>::max() >> _Bits << _Bits;
+};
----------------
zoecarver wrote:
> What's the reasoning behind shifting something forward and back? Shouldn't this always negate the other operation? 
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.


================
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();
----------------
zoecarver wrote:
> Maybe change `INFINITY` to `std::numeric_limits< _RealT >::infinity()`
Why isn't this just `__trunc_r > _MaxVal`?


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


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


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D66836