[libc++] Fix __wrap_iter to be a proper contiguous iterator.

[• Quuxplusone]

Summary

The latest installment in the ever-evolving saga of __cpp17_contiguous_iterator, __wrap_iter, and __unwrap_iter...

Instead of overloading __to_address, let's specialize pointer_traits. Function overloads need to be in scope at the point where they're called, whereas template specializations do not. (User code can provide pointer_traits specializations to be used by already-included library code, so obviously __wrap_iter can do the same.)

pointer_traits<__wrap_iter<It>> cannot provide pointer_to, because you generally cannot create a __wrap_iter without also knowing the identity of the container into which you're trying to create an iterator. I believe this is OK; contiguous iterators are required to provide to_address but *not* necessarily pointer_to.

I'm less sure about whether pointer_traits must provide rebind... but I don't see how it could possibly be implementable, so I think it'll be OK to omit it.

[jloser]

libcxx/include/__iterator/wrap_iter.h

300

Question: can you explain where the naming comes from/what it stands for? I see precedence for this naming in type_traits, so it's fine as-is given its use below, but just curious.

[• Quuxplusone]

libcxx/include/__iterator/wrap_iter.h

300

I believe __nat stands for "Not A Type."
The shape of this specialization was all just copied from the pointer_traits<_Tp*> specialization, btw.

[jloser]

Seems reasonable to me. Thanks for digging into this!

[ldionne]

This is much nicer, thanks!

Can you please add tests in test/libcxx for pointer_to? It's not clear to me that it works at all.

libcxx/include/__iterator/wrap_iter.h

287

I think we should only specialize for __wrap_iter<_Tp*>, or perhaps for any _It that is a contiguous iterator. Otherwise, we're not guaranteed that it's legal to call to_address(__w.base()) below. WDYT?

307

[• Quuxplusone]

Remove pointer_traits<__wrap_iter>::rebind and ::pointer_to.

[• Quuxplusone]

libcxx/include/__iterator/wrap_iter.h

287

We already assume that _It must be a contiguous iterator; see line 283, also line 36. This was the outcome of that whole series of PRs among Zoe and myself that finally sorted out that yes __wrap_iter only ever actually wraps pointers, and it would break horribly if you ever manually tried to wrap anything that wasn't a pointer, so let's just stop pretending that it can.

293–296

@ldionne wrote:

Can you please add tests in test/libcxx for pointer_to? It's not clear to me that it works at all.

You're right, it was totally busted — and rebind was busted worse! :P I'd just copied that code over without looking at it. It's gone now. I think, but am not sure, that this is still conforming. I don't think client code can conformingly rely on being able to look at pointer_traits<vector<int>::iterator>, let alone pointer_traits<vector<int>::iterator>::rebind<const int> or whatever. This pointer_traits is just an implementation detail that gets us the enabled std::to_address that we actually want to be client-visible.

[ldionne]

LGTM except for the weird includes, and also I'd like to have a test that exercises std::to_address for a __wrap_iter. I suspect the #include confusion started as an attempt to add such a test?

I don't need to see this again unless it changes significantly. Thanks a bunch for looking into that!

libcxx/include/__iterator/wrap_iter.h

287

Sorry, I meant to delete my comment before submitting the review. You're right, I remembered that after writing it.

293–296

Yeah.. I don't think that specialization of pointer_traits would be conforming for a user defined type, but we can do it cause we're the implementation.

libcxx/test/std/utilities/memory/pointer.traits/pointer_to.pass.cpp

20–23

Why those includes?

[• Quuxplusone]

libcxx/test/std/utilities/memory/pointer.traits/pointer_to.pass.cpp

20–23

Oops. I had originally added some test cases here for pointer_traits<std::vector<int>::iterator>::pointer_to etc., before eventually realizing "oh yeah, that'll never work." And then I reverted that part of the file, but forgot to revert the includes.

[• Quuxplusone]

libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp

13–16

@ldionne wrote:

also I'd like to have a test that exercises std::to_address for a __wrap_iter.

That's what this is... and now it passes in debug mode too. :) This suffices, right?

[ldionne]

LGTM with include fixes and green CI!

libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp

13–16

Oh, sorry, yeah. Went too fast.

[• Quuxplusone]

Remove superfluous includes. If buildkite likes this, I'll land it.

libcxx/include/__iterator/wrap_iter.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef _LIBCPP___ITERATOR_WRAP_ITER_H
 #define _LIBCPP___ITERATOR_WRAP_ITER_H
 
 #include <__config>
 #include <__debug>
 #include <__iterator/iterator_traits.h>
-#include <__memory/pointer_traits.h> // __to_address
+#include <__memory/pointer_traits.h>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
__wrap_iter<_Iter1> operator+(typename __wrap_iter<_Iter1>::difference_type __n, __wrap_iter<_Iter1> __x) _NOEXCEPT
     return __x;
 }
 
 #if _LIBCPP_STD_VER <= 17
 template <class _It>
 struct __is_cpp17_contiguous_iterator<__wrap_iter<_It> > : true_type {};
 #endif
 
-template <class _Iter>
-_LIBCPP_CONSTEXPR
-decltype(_VSTD::__to_address(declval<_Iter>()))
-__to_address(__wrap_iter<_Iter> __w) _NOEXCEPT {
+template <class _It>
+struct _LIBCPP_TEMPLATE_VIS pointer_traits<__wrap_iter<_It> >
+{
+    typedef __wrap_iter<_It> pointer;
+    typedef typename pointer_traits<_It>::element_type element_type;
+    typedef typename pointer_traits<_It>::difference_type difference_type;
+
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    static element_type *to_address(pointer __w) _NOEXCEPT {
         return _VSTD::__to_address(__w.base());
     }
+};
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___ITERATOR_WRAP_ITER_H

libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // <memory>
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 
-// TODO: We should enable this test in Debug mode once we fix __wrap_iter
-//       to be a proper contiguous_iterator.
-// UNSUPPORTED: LIBCXX-DEBUG-FIXME
-
 // template <class T> constexpr T* to_address(T* p) noexcept;
 // template <class Ptr> constexpr auto to_address(const Ptr& p) noexcept;
 
 #include <memory>
 
 #include <array>
 #include <cassert>
 #include <span>

libcxx/test/std/utilities/memory/pointer.traits/pointer_to.pass.cpp

 // struct pointer_traits<T*>
 // {
 //     static pointer pointer_to(<details>); // constexpr in C++20
 //     ...
 // };
 
 #include <memory>
 #include <cassert>
+#include <string>
+#include <string_view>
+#include <vector>
+
 #include "test_macros.h"
 
-#if TEST_STD_VER > 17
-constexpr
-#endif
-bool check() {
+TEST_CONSTEXPR_CXX20 bool test()
+{
     {
         int i = 0;
-        static_assert((std::is_same<int *, decltype(std::pointer_traits<int*>::pointer_to(i))>::value), "");
-        int* a = std::pointer_traits<int*>::pointer_to(i);
-        assert(a == &i);
+        static_assert(std::is_same<decltype(std::pointer_traits<int*>::pointer_to(i)), int*>::value, "");
+        assert(std::pointer_traits<int*>::pointer_to(i) == &i);
     }
     {
-        (std::pointer_traits<void*>::element_type)0;
+        int i = 0;
+        static_assert(std::is_same<decltype(std::pointer_traits<const int*>::pointer_to(i)), const int*>::value, "");
+        assert(std::pointer_traits<const int*>::pointer_to(i) == &i);
     }
     return true;
 }
 
-int main(int, char**) {
-    check();
+int main(int, char**)
+{
+    test();
 #if TEST_STD_VER > 17
-    static_assert(check(), "");
+    static_assert(test());
 #endif
 
+    {
+        // Check that pointer_traits<void*> is still well-formed, even though it has no pointer_to.
+        static_assert(std::is_same<std::pointer_traits<void*>::element_type, void>::value, "");
+        static_assert(std::is_same<std::pointer_traits<const void*>::element_type, const void>::value, "");
+        static_assert(std::is_same<std::pointer_traits<volatile void*>::element_type, volatile void>::value, "");
+    }
+
     return 0;
 }