[libcxx-commits] [PATCH] D59099: Integer and pointer types of 'midpoint' from P0811
Eric Fiselier via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Wed Mar 13 20:13:42 PDT 2019
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
More information about the libcxx-commits
mailing list