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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
