[libcxx-commits] [PATCH] D107671: [libcxx][ranges] Add `ranges::join_view`.

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 6 inline comments as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/__ranges/join_view.h:340
+  template<class _Range>
+  explicit join_view(_Range&&) -> join_view<views::all_t<_Range>>;
+
----------------
ldionne wrote:
> Please make sure you test the explicit-ness of this deduction guide with something like that:
> 
> ```
> join_view v = some_range; // should not compile
> join_view v(some_range); // should compile
> ```
I can't seem to figure out how to test this. It seems like both compile: https://godbolt.org/z/qefr1vxqz

I'll think some more and see if I can come up with something. 


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
ldionne wrote:
> This is a general comment not only applicable to this test. We seem to be missing coverage for a lot of range "topologies" (what I mean by that is an empty range, a range with 1 subrange, 2 subranges, an empty subrange, subranges of empty sizes, etc..).
> 
> I don't think it is reasonable to replicate each test for all topologies, but we should add tests with something like a random access range (const and non-const) checking all topologies. What I mean here is that I don't care if you don't test a range like `[[x], [], [y, z]]` with all combinations of archetypes, but you should test it at least with a few that are relevant (that probably is restricted to const and non-const).
Added four tests: children all have different sizes (0-4), parent is empty, parent size is one, parent and child size is one. Let me know what you think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107671