[libcxx-commits] [PATCH] D123466: [libcxx][ranges] add views::join adaptor object. added test coverage to join_view

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

This generally looks really good -- I don't have much to add TBH. Thanks a lot for increasing the coverage and being diligent in your testing, this is really valuable.

One thing I would suggest (for the future) would be to split large reviews into a few orthogonal patches. Each LWG issue could have been addressed on its own, same for the added testing and the `join` adaptor. That being said, don't split it off now since this has gotten reviewed. But in the future, splitting it into orthogonal patches when possible will generally make it easier for reviewers to give you good feedback quickly.

I don't have any blocking comments, so I'm happy when the other reviewers are. Thanks a lot for the contribution, this is great!



================
Comment at: libcxx/docs/Status/Cxx2bIssues.csv:47
 "`3473 <https://wg21.link/LWG3473>`__","Normative encouragement in non-normative note","November 2020","|Nothing To Do|","","|format|"
-"`3474 <https://wg21.link/LWG3474>`__","Nesting ``join_views`` is broken because of CTAD","November 2020","","","|ranges|"
+"`3474 <https://wg21.link/LWG3474>`__","Nesting ``join_views`` is broken because of CTAD","November 2020","Complete","15.0","|ranges|"
 "`3476 <https://wg21.link/LWG3476>`__","``thread`` and ``jthread`` constructors require that the parameters be move-constructible but never move construct the parameters","November 2020","",""
----------------



================
Comment at: libcxx/docs/Status/Cxx2bIssues.csv:59
 "`3495 <https://wg21.link/LWG3495>`__","``constexpr launder`` makes pointers to inactive members of unions usable","February 2021","|Nothing To Do|",""
-"`3500 <https://wg21.link/LWG3500>`__","``join_view::iterator::operator->()`` is bogus","February 2021","","","|ranges|"
+"`3500 <https://wg21.link/LWG3500>`__","``join_view::iterator::operator->()`` is bogus","February 2021","Complete","14.0","|ranges|"
 "`3502 <https://wg21.link/LWG3502>`__","``elements_view`` should not be allowed to return dangling reference","February 2021","","","|ranges|"
----------------



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:12-13
+
+// std::views::join
+#include <ranges>
+
----------------



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:74
+    auto jv2 = std::views::join(jv);
+    static_assert(std::same_as<std::ranges::range_reference_t<decltype(jv2)>, Foo&>);
+
----------------
Those can be simplified as `ASSERT_SAME_TYPE(std::ranges::range_reference_t<decltype(jv2)>, Foo&);`. Non-blocking.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/adaptor.pass.cpp:37
+concept CanBePiped = requires(View&& view, T&& t) {
+                       { std::forward<View>(view) | std::forward<T>(t) };
+                     };
----------------
philnik wrote:
> huixie90 wrote:
> > var-const wrote:
> > > Nit: I think concepts are usually indented 2 (sometimes 4) spaces in our code base, I'd move this line way left.
> > lol. I adopted your suggestion but arc linter put the indentation back. what is the command to disable arc linter?
> `--nolint`
Specifically, you can upload your patch with `arc diff --nolint`. That should really be the default given how noisy it is :-)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/end.pass.cpp:28
+
+// | ID | outer  | outer   | outer  | inner | inner   | inner  |     end()     |    end()     |
+// |    | simple | forward | common | l_ref | forward | common |               |    const     |
----------------
We haven't been doing this very often, but I think it's a really neat and concise way of making sure we're exhaustive. I think we should start doing this more often when it makes sense.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:80-82
 // TODO: remove these bogus operators
-constexpr bool operator==(const cpp17_input_iterator<const int*> &lhs, const int* rhs) { return base(lhs) == rhs; }
-constexpr bool operator==(const int* lhs, const cpp17_input_iterator<const int*> &rhs) { return base(rhs) == lhs; }
+constexpr bool operator==(const cpp17_input_iterator<const int*>& lhs, const int* rhs) { return base(lhs) == rhs; }
+constexpr bool operator==(const int* lhs, const cpp17_input_iterator<const int*>& rhs) { return base(rhs) == lhs; }
----------------
It should be sufficient to use `sentinel_wrapper` in order to remove those (and below).


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.join.view/types.h:372
+template <class Base>
+struct move_only_input_iter_with_arrow {
+  Base it_;
----------------
If some of those are only used in a single test file, I would consider moving them to that test file in order to keep this one smaller, and keep things as localized as possible. I think this is the case here, and IMO the test in `arrow.pass.cpp` would be a bit easier to understand if we defined this type in `arrow.pass.cpp`, closer to where it is used. For other types that are reused across multiple tests, I think it's worth avoiding code duplication of course. Non-blocking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123466