[libcxx-commits] [PATCH] D126053: [libc++][ranges] Finish LWG issues directly related to the One Ranges Proposal.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM with comments applied and CI passing.



================
Comment at: libcxx/include/__iterator/move_iterator.h:161
     reference operator*() const { return static_cast<reference>(*__current_); }
-    _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
+    _LIBCPP_DEPRECATED_IN_CXX20 _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX14
     pointer operator->() const { return __current_; }
----------------
Can we add a test for this deprecation? Example in `libcxx/test/std/utilities/memory/storage.iterator/deprecated.verify.cpp`.


================
Comment at: libcxx/include/__ranges/empty.h:31
 namespace __empty {
-  template <class _Tp>
-  concept __member_empty =
-    __workaround_52970<_Tp> &&
-    requires(_Tp&& __t) {
-      bool(__t.empty());
-    };
-
-  template<class _Tp>
-  concept __can_invoke_size =
-    !__member_empty<_Tp> &&
-    requires(_Tp&& __t) { ranges::size(__t); };
-
-  template <class _Tp>
-  concept __can_compare_begin_end =
-    !__member_empty<_Tp> &&
-    !__can_invoke_size<_Tp> &&
-    requires(_Tp&& __t) {
-      bool(ranges::begin(__t) == ranges::end(__t));
-      { ranges::begin(__t) } -> forward_iterator;
-    };
-
-  struct __fn {
-    template <__member_empty _Tp>
-    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool operator()(_Tp&& __t) const
-        noexcept(noexcept(bool(__t.empty()))) {
-      return bool(__t.empty());
-    }
-
-    template <__can_invoke_size _Tp>
-    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool operator()(_Tp&& __t) const
-        noexcept(noexcept(ranges::size(__t))) {
-      return ranges::size(__t) == 0;
-    }
-
-    template<__can_compare_begin_end _Tp>
-    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr bool operator()(_Tp&& __t) const
-        noexcept(noexcept(bool(ranges::begin(__t) == ranges::end(__t)))) {
-      return ranges::begin(__t) == ranges::end(__t);
-    }
+
+template <class _Tp>
----------------
Do you mind checking-in these formatting changes separately? No need for a review, mark it as NFC.

Doing it as a separate commit makes it less confusing when looking at `git blame` output, `git log` and it's also possible to ignore it in `.git-blame-ignore-revs` (or whatever it's called).


================
Comment at: libcxx/include/iterator:404
     constexpr reference operator*() const;
-    constexpr pointer operator->() const;
     constexpr move_iterator& operator++();
----------------
I would instead say

```
constexpr pointer operator->() const; // deprecated in C++20
```

And if they remove it at some point, I would say `// deprecated in C++20, removed in C++XY`.


================
Comment at: libcxx/test/std/ranges/range.access/begin.verify.cpp:12
+
+// std::ranges::begin
+
----------------
I think these tests are fine, however it would be nice to add a small comment explaining that we're ensuring that our implementation produces a diagnostic for this ill-formed case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126053