[libcxx-commits] [PATCH] D90999: [libc++] Implements ranges::enable_borrowed_range

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.

LGTM % comments. I suspect we will end up removing `_LIBCPP_INLINE_VISIBILITY` in the place it's commented-on, but that can be done in post.



================
Comment at: libcxx/include/span:525
+inline _LIBCPP_INLINE_VISIBILITY constexpr
+bool _VSTD::ranges::enable_borrowed_range<span<_Tp, _Extent>> = true;
+#endif // !defined(_LIBCPP_HAS_NO_RANGES)
----------------
Nit: I'd prefer not to see `_VSTD::` here.
Could someone (presumably @ldionne) comment on whether `_LIBCPP_INLINE_VISIBILITY` is a good idea here? My intuitive impression is "no, it shouldn't be here"; compare how we did it in `<numbers>`. (But those are partial specializations expressed via C++20 constraints, whereas this is a partial specialization expressed in the old-fashioned way. I don't know if that makes a subtle difference.)

(Note that `_LIBCPP_INLINE_VAR` is also a thing, but I think we don't want it here (because this is post-C++14-only) and my grepping didn't turn up any more precedents for what we're doing here with partial specialization.)


================
Comment at: libcxx/test/std/containers/views/enable_borrowed_range.compile.pass.cpp:46-52
+void test() {
+  test<char>();
+  test<int>();
+  test<double>();
+  test<S>();
+  test<char*>();
+}
----------------
This test is overly complicated. Please replace the whole thing with these five lines, which test everything you've got now //plus// the fact that cvref-qualification matters to the specialization //plus// will give better error messages in the extremely unlikely case that this test ever fails.
```
void test() {
    static_assert(std::ranges::enable_borrowed_range<std::span<int, 0>>);
    static_assert(std::ranges::enable_borrowed_range<std::span<int, 42>>);
    static_assert(std::ranges::enable_borrowed_range<std::span<int, std::dynamic_extent>>);
    static_assert(!std::ranges::enable_borrowed_range<std::span<int, 42>&>);
    static_assert(!std::ranges::enable_borrowed_range<std::span<int, 42> const>);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90999