[libcxx-commits] [PATCH] D135248: [libc++] implement move_iterator<T*> should be a random access iterator

[Hui via Phabricator via libcxx-commits]

huixie90 requested changes to this revision.
huixie90 added a comment.

Thanks for the patch. Please take a look at the test
libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/types.pass.cpp
and update it, and/or add new tests

Another question is it this paper a DR or not? do we want to DR back to C++20's or just for C++23?



================
Comment at: libcxx/include/__iterator/move_iterator.h:64
+   
+   static constexpr auto _Get_Iterator_Tag() {
+     if constexpr (__is_cpp17_random_access_iterator<_Iter>::value) {
----------------
philnik wrote:
> You can make this function `consteval`. IMO Something like `__get_iterator_concept()` would be clearer, since iterator tags are used for both `iterator_category` and `iterator_concept`, but this function is only correct for `iterator_concept`. You also have to guard the functions with `_LIBCPP_STD_VER >= 20`.
either add `_LIBCPP_HIDE_FROM_ABI` macro, or change the function to `consteval`


================
Comment at: libcxx/include/__iterator/move_iterator.h:65-69
+     if constexpr (__is_cpp17_random_access_iterator<_Iter>::value) {
+        return random_access_iterator_tag{};
+     } else if constexpr (__is_cpp17_forward_iterator<_Iter>::value) {
+        return forward_iterator_tag{};
+     } else if constexpr (__is_cpp17_bidirectional_iterator<_Iter>::value) {
----------------
philnik wrote:
> The paper is referencing C++20 iterators, not C++17 iterators.
According to p2520r0, these checks should check against C++20 concept rather than C++17 concepts
```
if constexpr (random_access_iterator<_Iter>){
 // ...
} else if constexpr (bidirectional_iterator<_Iter>) {
 // ...
} else if constexpr ( forward_iterator<_Iter>) {

```

And then the function should be guarded against C++ 20


================
Comment at: libcxx/include/__iterator/move_iterator.h:67
+        return random_access_iterator_tag{};
+     } else if constexpr (__is_cpp17_forward_iterator<_Iter>::value) {
+        return forward_iterator_tag{};
----------------
`forward_iterator` check should come after `bidirectional_iterator`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135248