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

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF requested changes to this revision.
EricWF added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__iterator/move_iterator.h:63
+    private:
+#if _LIBCPP_STD_VER > 17
+   static consteval auto _get_iterator_concept() {
----------------
As others have said `_LIBCPP_STD_VER > 20`


================
Comment at: libcxx/include/__iterator/move_iterator.h:64
+#if _LIBCPP_STD_VER > 17
+   static consteval auto _get_iterator_concept() {
+     if constexpr (random_access_iterator<_Iter>) {
----------------
Yeah, this doesn't need `_LIBCPP_HIDE_FROM_ABI`, but if you want it for consistency...

In fact we don't need to hide 70% of the stuff we hide, but /shrug


================
Comment at: libcxx/include/__iterator/move_iterator.h:64
+#if _LIBCPP_STD_VER > 17
+   static consteval auto _get_iterator_concept() {
+     if constexpr (random_access_iterator<_Iter>) {
----------------
EricWF wrote:
> Yeah, this doesn't need `_LIBCPP_HIDE_FROM_ABI`, but if you want it for consistency...
> 
> In fact we don't need to hide 70% of the stuff we hide, but /shrug
Reserved names are `__lower_case_after_two_underscores` or `_UpperCaseAfterOneUnderscore`. 

You either need two underscores or a single underscore followed by a capital letter.


================
Comment at: libcxx/include/__iterator/move_iterator.h:76
+#endif // _LIBCPP_STD_VER > 17
+template<class _It2> friend class move_iterator;
+
----------------
Why did this stuff get moved?


================
Comment at: libcxx/test/std/iterators/predef.iterators/move.iterators/move.iterator/types.pass.cpp:145
   test<contiguous_iterator<char*>>();
-  static_assert(std::is_same_v<typename std::move_iterator<forward_iterator<char*>>::iterator_concept, std::input_iterator_tag>);
-  static_assert(std::is_same_v<typename std::move_iterator<bidirectional_iterator<char*>>::iterator_concept, std::input_iterator_tag>);
----------------
This is the correct behavior for C++20, the change only applies to C++2b.
So don't change these tests, but instead add more tests for the new C++20 behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135248