[libcxx-commits] [PATCH] D123600: [libc++][ranges] Implement `views::take`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision as: ldionne.
ldionne added a comment.

You might also want to take a look at D115122 <https://reviews.llvm.org/D115122> for inspiration if you have not already -- just leaving this note since I stumbled upon a comment that suggested doing that on my `ranges::drop` review. In particular, you should look at this comment and make sure we're OK: https://reviews.llvm.org/D116950#inline-1160422.

Since I'm about to go OOO and I don't see any real blocking issues, I'm going to approve this -- I don't think I'd need to see this again after CI is passing and my comments have been addressed.



================
Comment at: libcxx/include/__fwd/string_view.h:1
+// -*- C++ -*-
+//===---------------------------------------------------------------------===//
----------------
philnik wrote:
> `__string_view/fwd.h` would make more sense I think. Same for `span`.
FWIW I don't really have a strong opinion. The only downside to this is that we might end up with a lot of one-header subdirectories if we start adding forward declarations to more classes, and putting everything under `__fwd/` would avoid that.


================
Comment at: libcxx/include/__ranges/take_view.h:47
 
 namespace ranges {
+
----------------
Can you land the indentation change as a NFC patch before this one? No need for a review, I'd just like to avoid mixing it with this patch.


================
Comment at: libcxx/include/__ranges/take_view.h:226
+struct __passthrough_type<subrange<_Iter, _Sent, _Kind>> {
+  // `_Iter` template parameter of `subrange` is always equivalent to `iterator_t<T>`, where `T` is a `subrange`.
+  using type = subrange<_Iter>;
----------------
I don't think this comment is necessary to keep around in the long run. I understand its purpose in the context of this review, however.


================
Comment at: libcxx/include/__ranges/take_view.h:234
+struct __fn {
+  // [range.take.overview] 2.1
+  template <class _Range, convertible_to<range_difference_t<_Range>> _Np>
----------------
philnik wrote:
> I would only use the stable names. The numbers bit-rot way to fast.
I think I agree -- we're guilty of this in other places and I think that was a mistake, in retrospect.


================
Comment at: libcxx/include/__ranges/take_view.h:255-256
+    noexcept(noexcept(__passthrough_type_t<_RawRng>(
+                              ranges::begin(std::forward<_Rng>(__rng)),
+                              ranges::begin(std::forward<_Rng>(__rng)) +
+                                  std::min<_Dist>(ranges::distance(std::forward<_Rng>(__rng)), std::forward<_Np>(__n))
----------------
It is not clear to me that we want to use `std::forward` here. Indeed, http://eel.is/c++draft/range.take#view does say `U(ranges​::​begin(E), ranges​::​begin(E) + std​::​min<D>(ranges​::​distance(E), F))` where `E` is the expression in `views::take(E, F)`. However, we generally want to avoid forwarding in more than one expression at a time because that sets us up for double-moves. In this case it should not matter since `begin` has to take either `T&` or `T const&`, but I think I'd drop the `forward` here. IIUC, the same is true for `ranges::distance`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123600