[libcxx-commits] [PATCH] D59099: Integer and pointer types of 'midpoint' from P0811

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.

LGTM minus inline comments.



================
Comment at: libcxx/include/numeric:551
+enable_if_t<is_pointer_v<_TPtr>, _TPtr>
+midpoint(_TPtr __a, _TPtr __b) noexcept
+{
----------------
mclow.lists wrote:
> EricWF wrote:
> > Why the SFINAE?
> > 
> > Just declare it `midpoint(_Tp*, _Tp*)` ?
> Because of this bad thing:
>     midpoint<int>(0, 0)
> returns a `int *`
> 
> Yes, I know that users are not supposed to do that, but this is easy prevention.
The `int` case isn't quite right, but `midpoint<long>(0, 0)` triggers the bug you mention.

Is there a test case?


================
Comment at: libcxx/include/numeric:529
+_LIBCPP_INLINE_VISIBILITY constexpr
+enable_if_t<is_integral_v<_Tp> && !is_same_v<bool, _Tp>, _Tp>
+midpoint(_Tp __a, _Tp __b) noexcept
----------------
TODO on the floating point overloads?


================
Comment at: libcxx/include/numeric:549
+template <class _TPtr>
+_LIBCPP_INLINE_VISIBILITY constexpr
+enable_if_t<is_pointer_v<_TPtr>, _TPtr>
----------------
This overload isn't `constexpr` in the paper.


================
Comment at: libcxx/include/numeric:553
+{
+    return __a + midpoint<ptrdiff_t>(0, __b - __a);
+}
----------------
`minpoint` needs to be qualified. 


================
Comment at: libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.integer.pass.cpp:31
+    static_assert(                  noexcept(std::midpoint(T(), T())), "");
+    static_assert(std::midpoint<T>(one, three) == two, "");
+    using limits = std::numeric_limits<T>;
----------------
There's no reason why `midpoint` needs to be declared as a template. This test is non-portable.




================
Comment at: libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.integer.pass.cpp:64
+    assert(std::midpoint(limits::max(), T(-6)) == limits::max()/2 - 2);
+    assert(std::midpoint(T(-6), limits::max()) == limits::max()/2 - 3);
+}
----------------
I think we need more `constexpr` test that try to tickle overflows and other boundary conditions to ensure we're `constexpr` in those cases as well.


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

https://reviews.llvm.org/D59099