[libcxx-commits] [PATCH] D94807: [libc++] Rationalize our treatment of contiguous iterators and __unwrap_iter().

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked an inline comment as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/algorithm:1642
+    static _LIBCPP_CONSTEXPR _Iter
+    __f(_Iter __i) _NOEXCEPT {
+        return __i;
----------------
ldionne wrote:
> Can you name this `__apply`? Or we could use `operator()`. Either seems closer to conventions than `__f`.
I definitely object to `operator()`; I prefer named functions whenever possible. I don't object to `__apply`; I just picked `__f` because it was short. (We don't seem to have any instances of either `__f` or `__apply` with this connotation in the codebase. We do have quite a few `__test`, but that name seems inappropriate here because this is a transformation, not a SFINAE test.)


================
Comment at: libcxx/include/iterator:444
+// C++20 to allow optimizations for users providing wrapped iterator types.
+struct _LIBCPP_TEMPLATE_VIS contiguous_iterator_tag : public random_access_iterator_tag {};
 
----------------
@ldionne wrote:
> Arthur, some of this stuff landed in C++20, so we can still do it, but in C++20 only. Are you OK with that?

Yes, I believe so, unless I run into a problem with that.
The important subtlety here remains, even if we ignore Chromium. The way I've got the code structured now, //in both C++03 and C++20//,
- The std::copy-to-memmove optimization happens only for iterators where `__unwrap_iter(it)` returns a native pointer `T*`.
- `__unwrap_iter(it)` returns native pointers only for contiguous iterators, i.e., `__is_cpp17_contiguous_iterator<It>::value` is true and `_VSTD::__to_address(it)` exists.
- We want the std::copy-to-memmove optimization to happen for `vector<int>::iterator` i.e. `__wrap_iter<int*>`.

So //even in C++03 mode//, where `std::contiguous_iterator_tag` does not exist, we must ensure that `__is_cpp17_contiguous_iterator<int*>::value && __is_cpp17_contiguous_iterator<__wrap_iter<int*>>::value`.

I think that's a good state of affairs, and I think I can physically make it happen. But if I come back and say "oops, actually that's physically impossible because...," then I'll ask to revisit this.


================
Comment at: libcxx/include/iterator:609
+struct __is_cpp17_contiguous_iterator : _And<
+    __has_iterator_category_convertible_to<_Tp, random_access_iterator_tag>,
+    _Or<
----------------
ldionne wrote:
> Isn't this check made redundant by the following check for `__has_iterator_category_convertible_to<_Tp, contiguous_iterator_tag>`?
It's not completely redundant — it rejects user-defined iterator types with `iterator_category=input_iterator_tag` and `iterator_concept=contiguous_iterator_tag`. However, I can't point to any such types in the wild, and I didn't write any unit tests for such types, so I agree — I'll remove the redundancy. Let someone put it back if they run into such a case in the wild.


================
Comment at: libcxx/include/iterator:618
+template <class _Up>
+struct __is_cpp17_contiguous_iterator<_Up*> : __has_iterator_category_convertible_to<_Up*, random_access_iterator_tag> {};
 
----------------
ldionne wrote:
> I take it that you're not using just `true_type` here in case someone specializes `iterator_traits<MyFoo*>` to have a non random-access `iterator_category`?
As the comment indicated, my intent had been to make sure this specialization didn't accidentally accept types like `int(*)()`, `void*`,... which are syntactically `_Up*` but not actually iterators.
However, writing tests for this revealed that if you try to ask `__is_cpp17_contiguous_iterator<void*>` it just blows up anyway, because instantiating `iterator_traits<void*>` is ill-formed.
So as above, I agree — I'll just do `true_type` here and let someone "fix" the corner case for real, if they ever run into it.

I hadn't realized that `__has_iterator_category_convertible_to` wasn't SFINAE-friendly. (It barfs on most non-iterator types.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94807