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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked an inline comment as done.
EricWF added inline comments.


================
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;
+};
----------------
scanon wrote:
> 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.
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.



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

https://reviews.llvm.org/D66836