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

[Hui via Phabricator via libcxx-commits]

huixie90 added inline comments.


================
Comment at: libcxx/include/__fwd/string_view.h:1
+// -*- C++ -*-
+//===---------------------------------------------------------------------===//
----------------
ldionne wrote:
> 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.
I am in favour of fwd folder. I don’t like to have too many files with the exact same name fwd.hpp 


================
Comment at: libcxx/include/__ranges/take_view.h:191
+template <class _Tp>
+constexpr bool __is_empty_view<empty_view<_Tp>> = true;
+
----------------
I remember that clangd always suggests me to add `inline` in those partial variable template specialisations. I don’t get why but those constexpr bool variable template in the standard  all have `inline`s


================
Comment at: libcxx/include/__ranges/take_view.h:235
+  // [range.take.overview] 2.1
+  template <class _Range, convertible_to<range_difference_t<_Range>> _Np>
+    requires __is_empty_view<remove_cvref_t<_Range>>
----------------
I think it should be the different type of `_Range&&` instead of `_Range`


================
Comment at: libcxx/include/__ranges/take_view.h:259
+                              )))
+    -> decltype(      __passthrough_type_t<_RawRng>(
+                              ranges::begin(std::forward<_Rng>(__rng)),
----------------
 question : the spec doesn’t say “if the expression is ill formed, goto otherwise clause”. do we need sfinae here? 


================
Comment at: libcxx/include/__ranges/take_view.h:306
+  template <class _Np>
+    requires constructible_from<decay_t<_Np>, _Np>
+  [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
----------------
Should the second _Np be _Np&& ?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp:51
+
+constexpr bool test() {
+  constexpr int N = 8;
----------------
Could you add a test for 
> If decltype((F)) does not model convertible_­to<D>, views​::​take(E, F) is ill-formed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123600