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