[libcxx-commits] [PATCH] D102028: [libcxx][ranges] Implement views::all.

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

Quuxplusone requested changes to this revision.
Quuxplusone added a subscriber: BRevzin.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__ranges/views_all.h:38
+    constexpr auto operator()(_Tp&& __t) const {
+      return _VSTD::move(__t);
+    }
----------------
tcanens wrote:
> This should be forward.
Here and throughout, [my understanding is that] "expression-equivalent" requires an appropriate `noexcept` clause. (Please check your other PRs as well.)


================
Comment at: libcxx/include/__ranges/views_all.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
For the name of this header, I think you can get away with `<__ranges/all.h>`, since the name of the entity is `all` (not `views_all`). Or is this going to pose some kind of problem when we get to some other specific view whose name is inappropriate and/or already taken?


================
Comment at: libcxx/include/__ranges/views_all.h:13-17
+#include <__iterator/iterator_traits.h>
+#include <__iterator/concepts.h>
+#include <__ranges/concepts.h>
+#include <__ranges/access.h>
+#include <__ranges/view.h>
----------------
alphabetize (also in view_interface.h)


================
Comment at: libcxx/include/__ranges/views_all.h:52
+    constexpr auto operator()(_Tp&& __t) const {
+      return ranges::subrange{_VSTD::forward<_Tp>(__t)};
+    }
----------------
This may be a complete red herring: I vaguely recall an LWG issue and/or paper that pointed out that a whole bunch of brace-initializations in the Standard needed to be parens-initializations, and/or needed to stop using CTAD, because they would do the wrong thing under CTAD. All I can actually find by googling/gmail-searching, though, is @brevzin's https://cplusplus.github.io/LWG/issue3474 . Does anyone know whether (1) such a blanket issue exists, (2) should exist, and/or (3) applies here? @tcanens, thoughts?

(This code looks correct-w.r.t.-the-standard-as-written, modulo `noexcept`.)


================
Comment at: libcxx/include/__ranges/views_all.h:58
+inline namespace __cpo {
+  inline constexpr const auto all = __all::__fn{};
+} // namespace __cpo
----------------
What's the point of `const` here? (I think "none, remove it," but I could be missing something subtle.)


================
Comment at: libcxx/include/__ranges/views_all.h:61
+
+}
+
----------------
`} // namespace views`


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp:21-22
+
+namespace ranges = std::ranges;
+namespace views = std::views;
+
----------------
`stdr`, and presumably `stdv`
(My personal preference is `rg` and `rv`, but `stdr` is already in the test suite.)
However, bonus points if you can just //not use namespace aliases//. It seems like you're already avoiding them in some cases, e.g. `std::ranges::view_base` on line 26. Can you just do that throughout? Makes it a lot easier to grep for test coverage, because `git grep std::views::all libcxx/test/` will actually return some hits.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp:85
+  return 0;
+}
----------------
You need tests that hit every different overload in the dispatch, and also significant CTAD cases. At least, test that `views::all(x)` has the same type as `x` when `x` is a `subrange`, and when `x` is a `ref_view`.
For the tests you've got now, I think it'd be better to do something that differentiates a copy-constructed `Range` from a default-constructed `Range`. For example, give `Range` a constructor `constexpr explicit Range(int *b, int *e) : b_(b), e_(e) {}`, then construct ```
    int i[2];
    auto range = Range(i, i+2);
    auto v = std::views::all(range);
    ASSERT_SAME_TYPE(decltype(v), std::ranges::ref_view<Range>);
    assert(std::ranges::begin(v) == i);
    assert(std::ranges::end(v) == i+2);
```
Test that `views::all(view_lvalue)` works just as well as `views::all(View())`.
Test that `views::all(Range())` SFINAEs away (I assume).
Test that `const auto range = Range(i, i+2); std::views::all(range)` gives you a `std::ranges::ref_view<const Range>` (I assume).
Maybe test with a range that is not a common_range (i.e., one with a sentinel type different from its iterator type), and that that gives you back a `subrange` with the appropriate template parameters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102028