[libc++] Fix ability to explicitly instantiate std::midpoint

[SanjayMarreddi]

std::midpoint is specified by having a pointer overload in [numeric.ops.midpoint].
With the way the pointer overload is specified, users can expect that calling
std::midpoint as std::midpoint<T>(a, b) should work, but it didn't in libc++
due to the way the pointer overload was specified.

Fixes #67046

[mordante]

Thanks for your patch!
In libc++ we tend to heavily test our code. Can you add tests of code that does not work now and starts to work after the patch. Best use the example of #67046.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Sanjay Marreddi (SanjayMarreddi)

Changes

Fixes #67046

---

Full diff: https://github.com/llvm/llvm-project/pull/74217.diff

3 Files Affected:

• (modified) libcxx/include/__numeric/midpoint.h (+3-10)
• (modified) libcxx/test/libcxx/numerics/numeric.ops/midpoint.integer.pass.cpp (+4-4)
• (modified) libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.pointer.pass.cpp (+6)

diff --git a/libcxx/include/__numeric/midpoint.h b/libcxx/include/__numeric/midpoint.h
index c92e450767c97d..e97f98f370979f 100644
--- a/libcxx/include/__numeric/midpoint.h
+++ b/libcxx/include/__numeric/midpoint.h
@@ -51,19 +51,12 @@ _LIBCPP_DISABLE_UBSAN_UNSIGNED_INTEGER_CHECK
     return __a + __half_diff;
 }
 
-
 template <class _TPtr>
-_LIBCPP_HIDE_FROM_ABI constexpr
-enable_if_t<is_pointer_v<_TPtr>
-             && is_object_v<remove_pointer_t<_TPtr>>
-             && ! is_void_v<remove_pointer_t<_TPtr>>
-             && (sizeof(remove_pointer_t<_TPtr>) > 0), _TPtr>
-midpoint(_TPtr __a, _TPtr __b) noexcept
-{
-    return __a + std::midpoint(ptrdiff_t(0), __b - __a);
+_LIBCPP_INLINE_VISIBILITY constexpr enable_if_t<is_object_v<_TPtr> && !is_void_v<_TPtr> && (sizeof(_TPtr) > 0), _TPtr*>
+midpoint(_TPtr* __a, _TPtr* __b) noexcept {
+  return __a + _VSTD::midpoint(ptrdiff_t(0), __b - __a);
 }
 
-
 template <typename _Tp>
 _LIBCPP_HIDE_FROM_ABI constexpr int __sign(_Tp __val) {
     return (_Tp(0) < __val) - (__val < _Tp(0));
diff --git a/libcxx/test/libcxx/numerics/numeric.ops/midpoint.integer.pass.cpp b/libcxx/test/libcxx/numerics/numeric.ops/midpoint.integer.pass.cpp
index 302948756b198c..ef559adda772f8 100644
--- a/libcxx/test/libcxx/numerics/numeric.ops/midpoint.integer.pass.cpp
+++ b/libcxx/test/libcxx/numerics/numeric.ops/midpoint.integer.pass.cpp
@@ -22,14 +22,14 @@
 
 //  Users are not supposed to provide template argument lists for
 //  functions in the standard library (there's an exception for min and max)
-//  However, libc++ protects against this for pointers, so we check to make
-//  sure that our protection is working here.
-//  In some cases midpoint<int>(0,0) might get deduced as the pointer overload.
+//  However, libc++ protects against this for pointers. The use of T(0)
+//  in the test cases resolves potential ambiguity in template argument deduction
+//  for the std::midpoint function.
 
 template <typename T>
 void test()
 {
-    ASSERT_SAME_TYPE(T, decltype(std::midpoint<T>(0, 0)));
+  ASSERT_SAME_TYPE(T, decltype(std::midpoint<T>(T(0), T(0))));
 }
 
 int main(int, char**)
diff --git a/libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.pointer.pass.cpp b/libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.pointer.pass.cpp
index 62ae099b458f23..5138fd6a37469f 100644
--- a/libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.pointer.pass.cpp
+++ b/libcxx/test/std/numerics/numeric.ops/numeric.ops.midpoint/midpoint.pointer.pass.cpp
@@ -54,6 +54,12 @@ void runtime_test()
     assert(std::midpoint(array +    9, array) == array + 5);
     assert(std::midpoint(array +   10, array) == array + 5);
     assert(std::midpoint(array +   11, array) == array + 6);
+
+    // explicit instantiation
+    ASSERT_SAME_TYPE(decltype(std::midpoint<T>(array, array)), T*);
+    ASSERT_NOEXCEPT(std::midpoint<T>(array, array));
+    assert(std::midpoint<T>(array, array) == array);
+    assert(std::midpoint<T>(array, array + 1000) == array + 500);
 }
 
 template <typename T>

[ldionne]

Thanks for the patch! I have a few small comments but this is close.

[ldionne]

Please use _LIBCPP_HIDE_FROM_ABI instead of _LIBCPP_INLINE_VISIBILITY -- they are equivalent but _LIBCPP_INLINE_VISIBILITY is going away.

[ldionne]

I would suggest renaming this template parameter to _Tp since now this is just a normal type, not a pointer anymore.

[ldionne]

Please use std:: instead of _VSTD::. I suspect those are incorrectly-resolved merge conflict artifacts.

[SanjayMarreddi]

Yeah true, updated it now.

[philnik777]

While we're modifying this code anyways, let's also refactor the enable_if_t to the enable_if_t<..., int> = 0 variant.

[SanjayMarreddi]

Sure, refactored it.

[ldionne]

Thanks, this LGTM. I edited your PR title and description since that is what the commit message is generated from. In the future, please try to make commit messages self-explanatory (within reason). I know GitHub PRs make it easier than ever to reference issues in commit messages, but it would be nice to keep the repository history somewhat decoupled from GitHub.

This can be merged once the CI is green, whoever sees it first.

[SanjayMarreddi]

Thanks for the review @ldionne. Sure, I will keep the suggestions in mind in my future PRs.

[philnik777]

@SanjayMarreddi It looks like you have enabled to hide your email on GitHub, which means that commits are authored as [email protected] . We're trying to avoid that to have proper attribution without relying on GitHub. Please disable the setting so we can merge the patch.

[SanjayMarreddi]

@philnik777 I have disabled the setting now. Please look into it now. Thanks!