[libcxx-commits] [PATCH] D101404: [libcxx] Fix __wrap_iter to work with to_address.

Wed Apr 28 12:59:05 PDT 2021

zoecarver added a comment.

@quuxplusone I didn't look at that patch, but now I have lots of thoughts 😁. First, after C++17 we should probably use `std::contiguous_iterator` to implement `__is_cpp17_contiguous_iterator`. Second, pre-C++20 AFAICT `__is_cpp17_contiguous_iterator` is the same as `is_pointer_v` + a specialization for `__iter_wrap`, right? I think we need to make this at least check for `pointer_traits` so that we can handle fancy pointers as well.

> Can we now remove the specialization of __is_cpp17_contiguous_iterator for __wrap_iter?

Can we just remove `__is_cpp17_contiguous_iterator` all together? It's only used in two places in the whole codebase.

> Can we now remove the overload of _VSTD::__to_address for __wrap_iter? I think no, but I'm not sure (and it sure would be nice if we could).

Not only can we, but we should. This is causing problems because it makes something work that really shouldn't. There's no reason `__wrap_iter` can't conform to `contiguous_iterator` via the standard method (by providing a template or element_type).

Side note: the reason we can't use the template to deduce the element type is that the template is a pointer (hence the "wrap" part of the name).

Anyway, simply providing `element_type` as this patch does is sufficient for `to_address` to start working.

> You're providing element_type unconditionally, which means that __wrap_iter<It> will be treated as pointer-like even when It is not a (fancy) pointer type itself; e.g. __wrap_iter<std::list<int>::iterator>. My impression is that we actually only ever use __wrap_iter to wrap (fancy) pointers — is this accurate? If it is not accurate, then more digging is warranted. If it is accurate, then we should document that invariant, and maybe even search-and-replace _Iter to _Ptr throughout __wrap_iter.

I think we could make this guarantee and I don't see any reason not to, especially if it makes our implementation simpler/better.

Note: the only place `static_assert(sizeof(typename pointer_traits<_Iter>::element_type), "")` fails in the whole codebase is in the `__is_cpp17_contiguous_iterator` test: `test/libcxx/iterators/contiguous_iterators.pass.cpp`.

================
Comment at: libcxx/include/iterator:1280
     typedef typename iterator_traits<iterator_type>::pointer           pointer;
+    typedef typename pointer_traits<pointer>::element_type             element_type;
     typedef typename iterator_traits<iterator_type>::reference         reference;
----------------
Quuxplusone wrote:
> If my comments are correct, then this could+should use `pointer_traits<_Iter>` instead of `pointer_traits<pointer>`...
> ...and the `_If` on line 1284 is unconditionally true, and can be simplified.
> Please try that simplification (at least locally) and see if it passes the test suite.
Alternatively we could add `element_type` with an `_If` (or maybe inheritance?) to make this work for non-pointer-like iterators. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101404