[libcxx][ranges] Implement views::all.

[zoecarver]

I think I need to write some more tests for this.

libcxx/include/__ranges/view_interface.h

31–32

Sorry about these random changes. I need to rebase the whole stack at some point (at which point these will disappear).

[tcanens]

libcxx/include/__ranges/views_all.h

38 ↗

(On Diff #343519)

This should be forward.

[zoecarver]

• Apply review comment.
• Rebase.

[• Quuxplusone]

libcxx/include/__ranges/views_all.h

1 ↗

(On Diff #344536)

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?

13–17 ↗

(On Diff #344536)

alphabetize (also in view_interface.h)

52 ↗

(On Diff #344536)

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.)

58 ↗

(On Diff #344536)

What's the point of const here? (I think "none, remove it," but I could be missing something subtle.)

61 ↗

(On Diff #344536)

} // namespace views

38 ↗

(On Diff #343519)

Here and throughout, [my understanding is that] "expression-equivalent" requires an appropriate noexcept clause. (Please check your other PRs as well.)

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp

22–23

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.

86

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.

[cjdb]

libcxx/include/__ranges/views_all.h

52 ↗

(On Diff #344536)

Yes, I think this is informally called Tim-initialisation-fiasco in LWG since he pointed it out. I distinctly remember something relevant in Cologne or Belfast, but I thought it was all resolved there. I've gotten into the habit of putting // braces intentional on anything that genuinely needs brace-init in template code.

38 ↗

(On Diff #343519)

Here and throughout, [my understanding is that] "expression-equivalent" requires an appropriate noexcept clause. (Please check your other PRs as well.)

Yep.

[tcanens]

libcxx/include/__ranges/views_all.h

51 ↗

(On Diff #344536)

This needs to be SFINAE friendly if the subrange construction is ill-formed.

52 ↗

(On Diff #344536)

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.

I think you are thinking about LWG3524 which will be resolved by P2367R0. The other braces in the ranges clause - including these - have no semantic effect and may be replaced with parens at editorial discretion: https://github.com/cplusplus/draft/issues/4593.

Yes, I think this is informally called Tim-initialisation-fiasco in LWG since he pointed it out. I distinctly remember something relevant in Cologne or Belfast, but I thought it was all resolved there.

That's a different one where the concept currently named default_initializable did not require T{} to work until LWG3149 was moved in Belfast, and so all the default member initializers needed to be T t = T(); instead of T t{};

[• Quuxplusone]

libcxx/include/__ranges/views_all.h

52 ↗

(On Diff #344536)

I've gotten into the habit of putting // braces intentional on anything that genuinely needs brace-init in template code.

I almost approve of that idea! (The problem I see with it is that I think the kind of person who'd put braces when they weren't needed is also the kind of person who'd put // braces intentional out of a misplaced sense of purpose. ;)) In a perfect world, it'd be useful to have a documented list of "reasons libc++ might use braces," and then the comment could be, like // braces for reason #2 on that list.
Here we have a double whammy: "braces intentional(?)" and "CTAD intentional(?)".

I would approve even more of a // CTAD intentional comment — which wouldn't really need to explain "why," because CTAD does like three things at once and the reason is always "because we want all those things to happen, I guess." Which is also why I'm even more suspicious of unintended CTAD.
https://quuxplusone.github.io/blog/2018/12/09/wctad/

@cjdb, for my own curiosity: Do you think it's feasible for a library to implement C++20 Ranges without ever using CTAD in its implementation? Like, could one realistically spell out the intended template arguments to subrange here? or is the CTAD hiding some really hairy and irreducible complexity? (Leave aside for the moment whether you think it'd be a good idea to remove CTAD from libc++'s implementation. Actually my kneejerk assumption is that it would not be pragmatic to remove, especially at this early stage, simply because the Standard gives reference implementations in terms of CTAD and so removing our CTAD would simply increase our chances of getting behavior subtly different from the Standard. But in a hypothetical Other Vendor's Implementation, would removing CTAD here and throughout be physically feasible?)

[• Quuxplusone]

libcxx/include/__ranges/views_all.h

52 ↗

(On Diff #344536)

"I think you are thinking about LWG3524 which will be resolved by P2367R0." — Ah. Yes. (The issue there, IIUC, is that foo{a, b} implies left-to-right evaluation of the side effects of a and b; an unsequenced function invocation like ranges::foo(a, b) can never be made expression-equivalent to it. So LWG3524 doesn't apply to foo{} or foo{a}, only to things with multiple arguments.)

[zoecarver]

Apply Arthur's comments.

[zoecarver]

@Quuxplusone I applied all your comments expect for the one about the test. I'll apply that one tomorrow, when I have the time :)

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp

23

Remove these now that they're no longer used.

[zoecarver]

libcxx/include/__ranges/views_all.h

52 ↗

(On Diff #344536)

@cjdb, for my own curiosity: Do you think it's feasible for a library to implement C++20 Ranges without ever using CTAD in its implementation? Like, could one realistically spell out the intended template arguments to subrange here? or is the CTAD hiding some really hairy and irreducible complexity? (Leave aside for the moment whether you think it'd be a good idea to remove CTAD from libc++'s implementation. Actually my kneejerk assumption is that it would not be pragmatic to remove, especially at this early stage, simply because the Standard gives reference implementations in terms of CTAD and so removing our CTAD would simply increase our chances of getting behavior subtly different from the Standard. But in a hypothetical Other Vendor's Implementation, would removing CTAD here and throughout be physically feasible?)

drop_view, for example, relies on CTAD like this:

template<class R>
    drop_view(R&&, range_difference_t<R>) -> drop_view<views::all_t<R>>;

So, I think the answer is, unfortunately, "no."

[zoecarver]

Add suggested tests.

[zoecarver]

Rebase.

[ldionne]

Implementation LGTM, seems like we can improve tests to some degree (my comment but also Arthur's comment which doesn't appear to have been addressed yet).

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp

86

We seem to be missing tests for noexcept-ness.

[zoecarver]

Add noexcept tests.

[ldionne]

I'd like to see CI passing, but still LGTM. @Quuxplusone are you still red on this?

[zoecarver]

@Quuxplusone friendly ping. I'm going to rebase this shortly. After the tests pass I'm planning to land it.

[• Quuxplusone]

Sure, I don't need to be red anymore.

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp

24

Please remove the default template parameter here: just template<bool IsNoexcept>. Otherwise, you run the risk of writing ViewImpl (CTAD) when you meant ViewImpl<false>. Be explicit.

38

Let's static_assert here on both specializations, unless there's a reason not to.

static_assert(std::ranges::view<ViewImpl<true>>);
static_assert(std::ranges::view<ViewImpl<false>>);

In fact, you might consider renaming ViewImpl to just View, merely to shorten the line lengths throughout. Ditto for CopyableViewImpl.

95–96

Not a blocker, but I will note for @ldionne's benefit that we have recently seen (in D103769) that merely testing top-level noexceptness is not sufficient to catch implementation bugs. What we actually want to test here is that if the foo operation throws an exception, that exception is correctly propagated all the way up to the top level. It's not sufficient to just test that the top level is not marked noexcept, because the implementor may have accidentally marked one of the intermediate levels noexcept. We need runtime tests to actually verify that the "exception pipe" from low level to top level is unbroken.

(They should not be marking intermediate levels noexcept, because that's often harmful and never helpful. But empirically I observe them doing it anyway.)
https://quuxplusone.github.io/blog/2018/06/12/attribute-noexcept-verify/
https://stackoverflow.com/questions/66459162/where-do-standard-library-or-compilers-leverage-noexcept-move-semantics-other-t

[zoecarver]

Rebase. This is just to get the CI going, I'll apply your comments shortly, Arthur.

[zoecarver]

• Rebase and fix header includes.
• Remove default template param.
• ViewImpl -> View.

libcxx/include/CMakeLists.txt

set(files
   __memory/shared_ptr.h
   __memory/temporary_buffer.h
   __memory/uninitialized_algorithms.h
   __memory/unique_ptr.h
   __mutex_base
   __node_handle
   __nullptr
   __ranges/access.h
+  __ranges/all.h
   __ranges/concepts.h
   __ranges/data.h
   __ranges/empty.h
   __ranges/enable_borrowed_range.h
   __ranges/view.h
   __ranges/size.h
   __ranges/subrange.h
   __ranges/ref_view.h

libcxx/include/__ranges/all.h

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+#ifndef _LIBCPP___RANGES_ALL_H
+#define _LIBCPP___RANGES_ALL_H
+
+#include <__config>
+#include <__iterator/concepts.h>
+#include <__iterator/iterator_traits.h>
+#include <__ranges/access.h>
+#include <__ranges/concepts.h>
+#include <__ranges/ref_view.h>
+#include <__ranges/subrange.h>
+#include <__ranges/view.h>
+#include <type_traits>
+
+#if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
+#pragma GCC system_header
+#endif
+
+_LIBCPP_PUSH_MACROS
+#include <__undef_macros>
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+
+namespace views {
+
+namespace __all {
+  struct __fn {
+    template<class _Tp>
+      requires ranges::view<decay_t<_Tp>>
+    constexpr auto operator()(_Tp&& __t) const
+      noexcept(noexcept(_VSTD::__decay_copy(_VSTD::forward<_Tp>(__t))))
+    {
+      return _VSTD::forward<_Tp>(__t);
+    }
+
+    template<class _Tp>
+      requires (!ranges::view<decay_t<_Tp>>) &&
+               requires (_Tp&& __t) { ranges::ref_view{_VSTD::forward<_Tp>(__t)}; }
+    constexpr auto operator()(_Tp&& __t) const
+      noexcept(noexcept(ranges::ref_view{_VSTD::forward<_Tp>(__t)}))
+    {
+      return ranges::ref_view{_VSTD::forward<_Tp>(__t)};
+    }
+
+    template<class _Tp>
+      requires (!ranges::view<decay_t<_Tp>> &&
+                !requires (_Tp&& __t) { ranges::ref_view{_VSTD::forward<_Tp>(__t)}; })
+    constexpr auto operator()(_Tp&& __t) const
+      noexcept(noexcept(ranges::subrange{_VSTD::forward<_Tp>(__t)}))
+    {
+      return ranges::subrange{_VSTD::forward<_Tp>(__t)};
+    }
+  };
+}
+
+inline namespace __cpo {
+  inline constexpr auto all = __all::__fn{};
+} // namespace __cpo
+
+} // namespace views
+
+#endif // !defined(_LIBCPP_HAS_NO_RANGES)
+
+_LIBCPP_END_NAMESPACE_STD
+
+_LIBCPP_POP_MACROS
+
+#endif // _LIBCPP___RANGES_ALL_H

libcxx/include/__ranges/concepts.h

namespace ranges {
   concept bidirectional_range = forward_range<_Tp> && bidirectional_iterator<iterator_t<_Tp> >;
 
   template <class _Tp>
   concept common_range = range<_Tp> && same_as<iterator_t<_Tp>, sentinel_t<_Tp> >;
 
   template <class _Tp>
   concept random_access_range =
       bidirectional_range<_Tp> && random_access_iterator<iterator_t<_Tp> >;
+
+  template<class _Range>
+  concept borrowed_range =
+      range<_Range> &&
+      (is_lvalue_reference_v<_Range> || enable_borrowed_range<remove_cvref_t<_Range>>);
+
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 // clang-format on
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP_RANGES_CONCEPTS_H

libcxx/include/__ranges/ref_view.h

 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 // clang-format off
 namespace ranges {
-
-  template<class _Range>
-  concept borrowed_range =
-      range<_Range> &&
-      (is_lvalue_reference_v<_Range> || enable_borrowed_range<remove_cvref_t<_Range>>);
-
-  template<class _Range>
-  concept sized_range = range<_Range> && requires(_Range& t) {
-    ranges::size(t);
-  };
-
   template<range _Range>
     requires is_object_v<_Range>
   class ref_view : public view_interface<ref_view<_Range>> {
     _Range *__range = nullptr;
 
 public:
     constexpr ref_view() noexcept = default;
 

libcxx/include/__ranges/size.h

struct __fn {
     }
   };
 }
 
 inline namespace __cpo {
   inline constexpr const auto ssize = __ssize::__fn{};
 } // namespace __cpo
 
+template<class _Range>
+concept sized_range = range<_Range> && requires(_Range& t) {
+  ranges::size(t);
+};
+
 } // namespace ranges
 
 // clang-format off
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 _LIBCPP_POP_MACROS
 
 #endif // _LIBCPP___RANGES_SIZE_H

libcxx/include/__ranges/subrange.h

 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 // clang-format off
 namespace ranges {
-
-  template<class _Range>
-  concept borrowed_range =
-      range<_Range> &&
-      (is_lvalue_reference_v<_Range> || enable_borrowed_range<remove_cvref_t<_Range>>);
-
-  template<class _Range>
-  concept sized_range = range<_Range> && requires(_Range& t) {
-    ranges::size(t);
-  };
-
   template<class _From, class _To>
   concept __convertible_to_non_slicing =
     convertible_to<_From, _To> &&
     // If they're both pointers, they must have the same element type.
     !(is_pointer_v<decay_t<_From>> &&
       is_pointer_v<decay_t<_To>> &&
       !same_as<remove_pointer_t<decay_t<_From>>, remove_pointer_t<decay_t<_To>>>);
 

libcxx/include/__ranges/view_interface.h

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 #ifndef _LIBCPP___RANGES_VIEW_INTERFACE_H
 #define _LIBCPP___RANGES_VIEW_INTERFACE_H
 
 #include <__config>
 #include <__iterator/iterator_traits.h>
 #include <__iterator/concepts.h>
 #include <__ranges/access.h>
 #include <__ranges/empty.h>
+#include <__ranges/view.h>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 
 _LIBCPP_PUSH_MACROS
 #include <__undef_macros>
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 #if !defined(_LIBCPP_HAS_NO_RANGES)
 
 namespace ranges {
 
 template<class _Tp>
 concept __can_empty = requires(_Tp __t) { ranges::empty(__t); };
 
 template<class _Derived>
   requires is_class_v<_Derived> && same_as<_Derived, remove_cv_t<_Derived>>
 class view_interface : public view_base {
   [[nodiscard]] constexpr _Derived& __derived() noexcept {
     return static_cast<_Derived&>(*this);
   }

libcxx/include/ranges

namespace std::ranges {
   template <class _Tp>
     concept common_range = see below;
 }
 
 */
 
 #include <__config>
 #include <__ranges/access.h>
+#include <__ranges/all.h>
 #include <__ranges/concepts.h>
 #include <__ranges/data.h>
 #include <__ranges/empty.h>
 #include <__ranges/enable_borrowed_range.h>
 #include <__ranges/view.h>
 #include <__ranges/size.h>
 #include <__ranges/subrange.h>
 #include <__ranges/ref_view.h>

libcxx/test/std/ranges/range.adaptors/range.all.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17
+// UNSUPPORTED: libcpp-no-concepts
+// UNSUPPORTED: gcc-10
+
+// std::views::all;
+
+#include <ranges>
+
+#include <cassert>
+#include "test_macros.h"
+#include "test_iterators.h"
+
+int globalBuff[8];
+
+struct View : std::ranges::view_base {
+  View() = default;
+  View(View&&) = default;
+  View& operator=(View&&) = default;
+  constexpr friend int* begin(View&) { return globalBuff; }
+  constexpr friend int* begin(View const&) { return globalBuff; }
+  constexpr friend int* end(View&) { return globalBuff + 8; }
+  constexpr friend int* end(View const&) { return globalBuff + 8; }
+};
+
+struct Range {
+  int start = 0;
+  constexpr friend int* begin(Range const& range) { return globalBuff + range.start; }
+  constexpr friend int* end(Range const&) { return globalBuff + 8; }
+  constexpr friend int* begin(Range& range) { return globalBuff + range.start; }
+  constexpr friend int* end(Range&) { return globalBuff + 8; }
+};
+
+struct BorrowableRange {
+  int start = 0;
+  constexpr friend int* begin(BorrowableRange const& range) { return globalBuff + range.start; }
+  constexpr friend int* end(BorrowableRange const&) { return globalBuff + 8; }
+  constexpr friend int* begin(BorrowableRange& range) { return globalBuff + range.start; }
+  constexpr friend int* end(BorrowableRange&) { return globalBuff + 8; }
+};
+
+template<>
+inline constexpr bool std::ranges::enable_borrowed_range<BorrowableRange> = true;
+
+constexpr bool test() {
+  {
+    ASSERT_SAME_TYPE(decltype(std::views::all(View())), View);
+
+    auto viewCopy = std::views::all(View());
+    ASSERT_SAME_TYPE(decltype(viewCopy), View);
+    assert(std::ranges::begin(viewCopy) == globalBuff);
+  }
+
+  {
+    Range range;
+    auto ref = std::views::all(range);
+    ASSERT_SAME_TYPE(decltype(ref), std::ranges::ref_view<Range>);
+    assert(std::ranges::begin(ref) == globalBuff);
+  }
+
+  {
+    auto subrange = std::views::all(BorrowableRange());
+    ASSERT_SAME_TYPE(decltype(subrange), std::ranges::subrange<int*>);
+    assert(std::ranges::begin(subrange) == globalBuff);
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}