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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I like this - I find it cleaner than a "private contract" with some specific users that they are allowed to overload `__unwrap_iter`. But I'd like to know why can't we leave that only enabled in C++20 mode. @rnk Would it be acceptable for Chrome to compile with C++20? It seems like an optimization that would justify the work to upgrade?

As a matter of policy, I'm a bit reluctant to say "let's add this as an extension because our very important client Chrome is using it". I don't think it's a great way to make decisions. The only reason why I could see us doing it here is because Chrome previously had a free-pass with `__unwrap_iter` and we broke them. But my preference would be to avoid adding those extensions pre-C++20. @rnk can you comment?



================
Comment at: libcxx/include/algorithm:1642
+    static _LIBCPP_CONSTEXPR _Iter
+    __f(_Iter __i) _NOEXCEPT {
+        return __i;
----------------
Can you name this `__apply`? Or we could use `operator()`. Either seems closer to conventions than `__f`.


================
Comment at: libcxx/include/iterator:609
+struct __is_cpp17_contiguous_iterator : _And<
+    __has_iterator_category_convertible_to<_Tp, random_access_iterator_tag>,
+    _Or<
----------------
Isn't this check made redundant by the following check for `__has_iterator_category_convertible_to<_Tp, contiguous_iterator_tag>`?


================
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> {};
 
----------------
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`?


================
Comment at: libcxx/include/iterator:789-790
+    typedef _If<__is_cpp17_random_access_iterator<_Iter>::value,
+        random_access_iterator_tag,
+        bidirectional_iterator_tag>                          iterator_concept;
 
----------------
A test should be added for that.


================
Comment at: libcxx/include/iterator:1241-1244
+    typedef _If<__is_cpp17_random_access_iterator<_Iter>::value,
+        random_access_iterator_tag,
+        typename iterator_traits<_Iter>::iterator_category>  iterator_category;
+    typedef input_iterator_tag                               iterator_concept;
----------------
A test should be added for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94807