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

[Nikolas Klauser via Phabricator via libcxx-commits]

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

In D135248#3836234 <https://reviews.llvm.org/D135248#3836234>, @phyBrackets wrote:

> I'm not sure about, where to look for testing of it or need to create new tests ?

There should be a test in `libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator`.

Please go through https://libcxx.llvm.org/Contributing.html#pre-commit-check-list again and check that you did everything.



================
Comment at: libcxx/include/__iterator/move_iterator.h:64
+   
+   static constexpr auto _Get_Iterator_Tag() {
+     if constexpr (__is_cpp17_random_access_iterator<_Iter>::value) {
----------------
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`.


================
Comment at: libcxx/include/__iterator/move_iterator.h:65
+   static constexpr auto _Get_Iterator_Tag() {
+     if constexpr (__is_cpp17_random_access_iterator<_Iter>::value) {
+        return random_access_iterator_tag{};
----------------
The paper is referencing C++20 iterators, not C++17 iterators.


================
Comment at: libcxx/include/__iterator/move_iterator.h:69
+        return forward_iterator_tag{};
+     } else if constexpr (__is_cpp17_bidirectional_iterator<_Iter>::value) {
+        return bidirectional_iterator_tag{};
----------------
You have to check `bidirectional_iterator` before `forward_iterator`, since `random_access_iterator` is a subset of `bidirectional_iterator` is a subset of `forward_iterator`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135248