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

Tue Apr 19 10:27:55 PDT 2022

var-const added a comment.

In D123600#3450020 <https://reviews.llvm.org/D123600#3450020>, @ldionne wrote:

> In particular, you should look at this comment and make sure we're OK: https://reviews.llvm.org/D116950#inline-1160422.

There is a test case for a fixed-size `span`:

  // `views::take(span, n)` returns a `span` with a dynamic extent, regardless of the input `span`.
  {
    std::span<int, 8> s(buf);
    std::same_as<std::span<int, std::dynamic_extent>> decltype(auto) result = s | std::views::take(3);
    assert(result.size() == 3);
  }

================
Comment at: libcxx/docs/Status/RangesIssues.csv:15
 `P1716R3 <https://wg21.link/P1716R3>`__,Range Comparison Algorithms Are Over-Constrained,,
-`P1739R4 <https://wg21.link/P1739R4>`__,Avoiding Template Bloat For Ranges,,
+`P1739R4 <https://wg21.link/P1739R4>`__,Avoiding Template Bloat For Ranges,|Complete|,15.0
 `P1862R1 <https://wg21.link/P1862R1>`__,Range Adaptors For Non-Copyable Iterators,,
----------------
ldionne wrote:
> Note that this is technically not quite complete until we ship `ranges::drop`.
Great point, thanks. Removed for now.

================
Comment at: libcxx/include/__fwd/string_view.h:1
+// -*- C++ -*-
+//===---------------------------------------------------------------------===//
----------------
huixie90 wrote:
> philnik wrote:
> > huixie90 wrote:
> > > 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 
> > I don't like having a `__fwd` subdirectory because that would mean we have a single header spread across multiple subdirecories. That makes it a lot harder to see what is part of a header.
> I think having a single fwd folder whose subdirectories hierarchies follow the non-fwd hierarchies is pretty neat. You can easily find the forward declarations by adding a fwd at the beginning. One example code base is Boost.Hana 
I'm slightly in favor of having all the forward declarations under a single folder because it would make it easier to see what can and cannot be forward-declared. While having contents of a single header spread across several directories in general would be bad, I think splitting out forward declarations following a consistent pattern shouldn't cause any issues. @philnik How strongly do you feel about this?

================
Comment at: libcxx/include/__fwd/string_view.h:17
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
----------------
philnik wrote:
> 
Thanks!

================
Comment at: libcxx/include/__fwd/string_view.h:20-35
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+template<class _CharT, class _Traits = char_traits<_CharT> >
+class _LIBCPP_TEMPLATE_VIS basic_string_view;
+
+typedef basic_string_view<char>     string_view;
+#ifndef _LIBCPP_HAS_NO_CHAR8_T
----------------
jloser wrote:
> philnik wrote:
> > Shouldn't `string_view` be C++17 or later?
> Technically yes, but for `libc++`, no. We support `string_view` in older standards as an extension. 
`string_view` is not guarded on the language version.

================
Comment at: libcxx/include/__ranges/take_view.h:191
+template <class _Tp>
+constexpr bool __is_empty_view<empty_view<_Tp>> = true;
+
----------------
huixie90 wrote:
> 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
IIUC, this is to prevent potential ODR violations. Added `inline`, thanks.

================
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>;
----------------
ldionne wrote:
> 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.
Yeah, I guess it would have made more sense as a review comment. Removed.

================
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>
----------------
ldionne wrote:
> 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.
Thanks, this is a good point, but there doesn't seem to be a great alternative. Added short descriptions instead.

================
Comment at: libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp:51
+
+constexpr bool test() {
+  constexpr int N = 8;
----------------
huixie90 wrote:
> Could you add a test for 
> > If decltype((F)) does not model convertible_to<D>, views::take(E, F) is ill-formed
This is checked by
```
      static_assert(!CanBePiped<SomeView&,   decltype(std::views::take(/*n=*/NotAView{}))>);
```
Would you prefer a more tailored test? (e.g. rather than having an empty type, come up with a more plausible number type that isn't convertible?)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123600