[libcxx-commits] [PATCH] D106840: [libcxx][ranges] Add `std::ranges::single_view`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

LGTM with all comments applied and CI passing.



================
Comment at: libcxx/include/__ranges/single_view.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
cjdb wrote:
> ldionne wrote:
> > Could you add `views::single` at the same time? It's pretty small so it makes sense to implement both in the same go, I think.
> > 
> > Also, this makes me think that we don't have `views::empty`, right?
> If you're going to take @ldionne up on this (and I think you should), please DM me so we can discuss a cohesive design for the adaptor closures. I don't want us to have wildly different ideas about what goes in this namespace.
`views::single` is not a range adaptor object, so we still are not implementing the pipe operator. But regardless, let's chat on Discord since you seem to have a specific approach in mind for the pipe operator.


================
Comment at: libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp:1
+// Test that this uses copyable box.
+//===----------------------------------------------------------------------===//
----------------
Stray comment?


================
Comment at: libcxx/test/std/ranges/range.factories/range.single.view/assign.pass.cpp:10
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
----------------
I really like that you're testing this property of the type, thanks for being thorough.


================
Comment at: libcxx/test/std/ranges/range.factories/range.single.view/size.pass.cpp:13
+
+// static constexpr size_t size() noexcept;
+
----------------
cjdb wrote:
> ldionne wrote:
> > Since the spec documents this as being `static`, can you add a test that `std::ranges::single_view<int>::size()` works as well?
> Unless there are already tests in `ranges::size` for static members, is it worth adding a test here for `ranges::size` too?
I'm not sure I understand your comment. Are you asking that we should add a test for `ranges::size()` on a type that provides a static `size()` method? If that's your suggestion, I would agree. It's actually a hole in our current testing coverage, since the spec for `ranges::size()` basically says "use `x.size()` if that is valid", which does not specify whether `.size()` is a static or non-static member function.

So as a fly-by fix in this patch, @zoecarver  can you add a test in `ranges::size()` with a type that defines a `static` `size()` function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106840