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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Seems like a very useful function. `__max_representable_int_for_float` also seems useful. Should this work in C++03? If so there are a few changes that need to be made. It would also be great if this could be a `constexpr` (but, obviously, not necessary).



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


================
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;
----------------
Nit: maybe qualify all the uses of `numeric_limits` and similar?


================
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;
----------------
What is the enum providing for you? Couldn't this just be `static const int _Bits  = ...`?


================
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;
+};
----------------
What's the reasoning behind shifting something forward and back? Shouldn't this always negate the other operation? 


================
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;
----------------
This will not work before C++11.


================
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();
----------------
Maybe change `INFINITY` to `std::numeric_limits< _RealT >::infinity()`


================
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
----------------
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`


================
Comment at: test/libcxx/numerics/truncating_cast.pass.cpp:25
+  struct {
+    double Input;
+    IntT Expect;
----------------
Maybe test with more than just `double`. `float`, `long double`,  others?


================
Comment at: test/libcxx/numerics/truncating_cast.pass.cpp:28
+    bool IsRepresentable;
+  } TestCases[] = {
+      {0, 0, true},
----------------
C++03 will not like this :P


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D66836