[libc++] Add the std::views::common range adaptor

[• Quuxplusone]

LGTM mod comments, all of which are nits, I think.

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp

33

It would be good to test a non-view type also (i.e. one where all_t<R> isn't R). Something like [UNTESTED]

std::vector<int> v = {1, 2, 3};
auto r = std::views::common(v);
ASSERT_SAME_TYPE(decltype(r), std::ranges::ref_view<std::vector<int>>);

89

Also

static_assert(!CanBePiped<std::vector<int>, decltype(std::views::common)>);

or perhaps replace line 88 with

static_assert( CanBePiped<int(&)[10], decltype(std::views::common)>);
static_assert(!CanBePiped<int(&&)[10], decltype(std::views::common)>);
static_assert(!CanBePiped<int, decltype(std::views::common)>);

94

Not only that, but std::addressof(std::views::common) == std::addressof(std::ranges::views::common).
I don't know if we really care about testing this; but if we do, then perhaps we should test all the members of namespace views, in one single test file, together.

libcxx/test/std/ranges/range.adaptors/range.common.view/types.h

112

If this noexcept is needed, something is wonky. Ditto on line 124.

116

For my own information: Are the multiple overloads actually needed? I do have some vague recollection that they might be, for some poison-pill-related reason, but if so, geez, that's awful. :P

[jloser]

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp

12 ↗

(On Diff #374901)

Question: since this is a libc++ extension, should we move this file into libcxx/test/libcxx for example?

[ldionne]

Address review comments. Thanks!

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp

12 ↗

(On Diff #374901)

Yes, the convention would be to put those in libcxx/test/libcxx. However, I've been sort of on the fence about this recently, it feels easier and clearer to put those alongside the normal tests and just use REQUIRES: libc++ to express that it's a libc++ specific test. The benefits I see are that you don't have to search for tests in two directories, and also there's no risk of the folder hierarchies getting out of sync if they get moved around.

I'll move it to libcxx/test/libcxx if you request it, otherwise I'll leave as-is.

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp

33

Good suggestion, done. I used <array> because it's constexpr friendly.

89

I'm curious to understand why int(&&)[10] can't be piped?

94

We're not supposed to be able to take the address of those so I think it's reasonable not to test for that.

libcxx/test/std/ranges/range.adaptors/range.common.view/types.h

116

Yes, they are necessary for the poison pill thing, and yes, it's awful.

[jloser]

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp

12 ↗

(On Diff #374901)

FWIW, I'm OK leaving it where it is now alongside the other tests. I prefer that, but will point out we have some inconsistency it seems. We can clean it up at some point in the future if desired (I'd be in favor of it).

[Mordante]

libcxx/include/__ranges/common_view.h

112

Shouldn't there be tests to verify the noexcept status of the adaptor?

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp

12 ↗

(On Diff #374901)

But being a verify test already implies a libc++ test. I've seen those more often in the std directory.

[ldionne]

Fix modules CI issue

[• Quuxplusone]

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp

12 ↗

(On Diff #374901)

@ldionne:

I've been sort of on the fence about this recently, it feels easier and clearer to put those alongside the normal tests and just use REQUIRES: libc++ to express that it's a libc++ specific test

@Mordante:

But being a verify test already implies a libc++ test.

I think Louis's point makes sense; I'm fine with leaving .verify.cpp tests in the test/std/ directory, as long as the test runner — and all human contributors! — will understand that .verify.cpp implies "run only with Clang, and only with libc++."
I have one question about the proposed-new-status-quo, and two very minor items to put in the "against" column, just to show I've thought of them and rejected them:

• Question: In the proposed-new-status-quo, since .verify.cpp implies "libc++ and Clang," isn't REQUIRES: libc++ redundant here? I think you should remove it.
• Minor con: Contributors often add new tests by grabbing an existing test and modifying it. Putting .verify.cpp tests next to .pass.cpp tests might make it slightly more likely that a contributor will accidentally grab and modify the wrong kind of test. (But this point is pretty weak.)
• Minor con: Each .verify.cpp test is kind of like a little TODO, because it identifies test coverage that is present for Clang but missing for other compilers. Each test/libcxx/ test is kind of like a little TODO, too, because it identifies test coverage for features that are in libc++ but lack standards-conformance for one reason or another. So putting all these little TODO items in one place might make it easier to see how big is the "backlog" or "gap" between libc++ and full conformance. (But find test/ -name *.verify.cpp is just as easy to type as find test/libcxx/, so this point is even weaker.)

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp

61

Why introduce this typedef? Couldn't you just use NonCommonView throughout, below?

76–77

Just kvetching for the record: I see no benefit to this newfangled approach over the old-school no-Result-typedef approach of

auto result = partial(view);
ASSERT_SAME_TYPE(decltype(result), std::ranges::common_view<std::ranges::transform_view<SomeView, decltype(f)>>);

[• Quuxplusone]

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp

89

Assuming I'm correct that int(&&)[10] can't be piped :) my intuition is that rvalues of non-borrowed-range types can never be piped. int(&&)[10] is not an lvalue reference type, and int[10] is not a borrowed-range type; Q.E.D.

We aren't used to thinking about array rvalues in C++, but they're just like any other rvalue. See https://quuxplusone.github.io/blog/2021/08/07/p2266-field-test-results/#libreoffice-ostring for another example from like a month ago.

94

I agree; but I meant that right now you're just testing that views::x and ranges::views::x have the same type; you're not actually testing that they are two different names for the same object (which is what we want).
You can definitely pass std::views::common by reference to a function — that is a deliberate supported use-case — so you can get its address by using addressof on that reference. I don't know where we draw the line between "manipulating an object by reference such that you obtain a pointer to the object" (which is fine) and "taking the object's address" (which is allegedly forbidden). ;)
Anyway, minor point. I still think that if we're spending brain cells on this, we should do it for all the views, in one place, not scattered. And if we're not spending brain cells on it, then ship it. :)

[Mordante]

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp

12 ↗

(On Diff #374901)

Hmm I see my post was a bit terse. But I meant:

• Keep the file at its current location.
• Remove the REQUIRES: libc++

[ldionne]

Address review comments, in particular fix the confusion with .verify.cpp.

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp

12 ↗

(On Diff #374901)

Okay, reading this discussion made me change my mind, I guess. There are some misconceptions. verify.cpp tests are *not* libc++ specific in spirit. verify.cpp tests are simply a type of test supported by the testing format, and I don't think it makes sense to tie them to a specific implementation of the C++ stdlib.

Also, they are not Clang specific *in spirit*. It is true that we don't support them with any other compiler right now because we need a compiler that supports clang-verify, but in theory it would be conceivable to support verify.cpp tests for other compilers. There would also be issues with how to translate diagnostics and all, but my point is that in spirit there's nothing Clang or libc++ specific with those tests.

So what I'm going to do is move those libc++ specific tests to libcxx/test/libcxx, and also do a pass at our existing tests to move the libc++ specific ones to the right location.

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp

61

Cause I wanted to capture the fact that it's just "some view" -- I didn't pick NonCommonView specifically, it could have been a CommonView too.

76–77

In this case you're right because Result = ..... is long so I put it on its own line, but in general it does reduce duplication in the tests and it can be easier to read. I'm not strongly attached to it, but I kind of liked it when (I think) @cjdb mentioned it to me, so I started adopting it. I'll keep doing that unless we come to an agreement that it's inferior.

94

I'll colocate those tests in an upcoming patch.

[Mordante]

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.cpp

12 ↗

(On Diff #374901)

Thanks for the clarification and moving the appropriate tests.

libcxx/include/__ranges/common_view.h

 
 #include <__config>
 #include <__iterator/common_iterator.h>
 #include <__iterator/iterator_traits.h>
 #include <__ranges/access.h>
 #include <__ranges/all.h>
 #include <__ranges/concepts.h>
 #include <__ranges/enable_borrowed_range.h>
+#include <__ranges/range_adaptor.h>
 #include <__ranges/size.h>
 #include <__ranges/view_interface.h>
+#include <__utility/forward.h>
 #include <__utility/move.h>
 #include <concepts>
 #include <type_traits>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #pragma GCC system_header
 #endif
 

 
 template<class _Range>
 common_view(_Range&&)
   -> common_view<views::all_t<_Range>>;
 
 template<class _View>
 inline constexpr bool enable_borrowed_range<common_view<_View>> = enable_borrowed_range<_View>;
 
+namespace views {
+namespace __common {
+  struct __fn : __range_adaptor_closure<__fn> {
+    template<class _Range>
+      requires common_range<_Range>
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+    constexpr auto operator()(_Range&& __range) const
+      noexcept(noexcept(views::all(_VSTD::forward<_Range>(__range))))
+      -> decltype(      views::all(_VSTD::forward<_Range>(__range)))
+      { return          views::all(_VSTD::forward<_Range>(__range)); }
+
+    template<class _Range>
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+    constexpr auto operator()(_Range&& __range) const
+      noexcept(noexcept(common_view{_VSTD::forward<_Range>(__range)}))
+      -> decltype(      common_view{_VSTD::forward<_Range>(__range)})
+      { return          common_view{_VSTD::forward<_Range>(__range)}; }
+  };
+}
+
+inline namespace __cpo {
+  inline constexpr auto common = __common::__fn{};
+}
+} // namespace views
 } // namespace ranges
 
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___RANGES_COMMON_VIEW_H

libcxx/test/libcxx/ranges/range.adaptors/range.common.view/adaptor.nodiscard.verify.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: libcpp-has-no-incomplete-ranges
+
+// Test the libc++ extension that std::views::common is marked as [[nodiscard]] to avoid
+// the potential for user mistakenly thinking they're calling an algorithm.
+
+#include <ranges>
+
+void test() {
+  int range[] = {1, 2, 3};
+
+  std::views::common(range); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+  range | std::views::common; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+}

libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.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: libcpp-has-no-incomplete-ranges
+
+// std::views::common
+
+#include <ranges>
+
+#include <array>
+#include <cassert>
+#include <concepts>
+#include <utility>
+
+#include "test_iterators.h"
+#include "types.h"
+
+template <class View, class T>
+concept CanBePiped = requires (View&& view, T&& t) {
+  { std::forward<View>(view) | std::forward<T>(t) };
+};
+
+constexpr bool test() {
+  int buf[] = {1, 2, 3};
+
+  // views::common(r) is equivalent to views::all(r) if r is a common_range
+  {
+    {
+      CommonView view(buf, buf + 3);
+      std::same_as<CommonView> auto result = std::views::common(view);
+      assert(result.begin_ == buf);
+      assert(result.end_ == buf + 3);
+    }
+    {
+      using NotAView = std::array<int, 3>;
+      NotAView arr = {1, 2, 3};
+      std::same_as<std::ranges::ref_view<NotAView>> auto result = std::views::common(arr);
+      assert(result.begin() == arr.begin());
+      assert(result.end() == arr.end());
+    }
+  }
+
+  // Otherwise, views::common(r) is equivalent to ranges::common_view{r}
+  {
+    NonCommonView view(buf, buf + 3);
+    std::same_as<std::ranges::common_view<NonCommonView>> auto result = std::views::common(view);
+    assert(result.base().begin_ == buf);
+    assert(result.base().end_ == buf + 3);
+  }
+
+  // Test that std::views::common is a range adaptor
+  {
+    using SomeView = NonCommonView;
+
+    // Test `v | views::common`
+    {
+      SomeView view(buf, buf + 3);
+      std::same_as<std::ranges::common_view<SomeView>> auto result = view | std::views::common;
+      assert(result.base().begin_ == buf);
+      assert(result.base().end_ == buf + 3);
+    }
+
+    // Test `adaptor | views::common`
+    {
+      SomeView view(buf, buf + 3);
+      auto f = [](int i) { return i; };
+      auto const partial = std::views::transform(f) | std::views::common;
+      using Result = std::ranges::common_view<std::ranges::transform_view<SomeView, decltype(f)>>;
+      std::same_as<Result> auto result = partial(view);
+      assert(result.base().base().begin_ == buf);
+      assert(result.base().base().end_ == buf + 3);
+    }
+
+    // Test `views::common | adaptor`
+    {
+      SomeView view(buf, buf + 3);
+      auto f = [](int i) { return i; };
+      auto const partial = std::views::common | std::views::transform(f);
+      using Result = std::ranges::transform_view<std::ranges::common_view<SomeView>, decltype(f)>;
+      std::same_as<Result> auto result = partial(view);
+      assert(result.base().base().begin_ == buf);
+      assert(result.base().base().end_ == buf + 3);
+    }
+
+    // Check SFINAE friendliness
+    {
+      struct NotAView { };
+      static_assert(!std::is_invocable_v<decltype(std::views::common)>);
+      static_assert(!std::is_invocable_v<decltype(std::views::common), NotAView>);
+      static_assert( CanBePiped<SomeView&,   decltype(std::views::common)>);
+      static_assert( CanBePiped<int(&)[10],  decltype(std::views::common)>);
+      static_assert(!CanBePiped<int(&&)[10], decltype(std::views::common)>);
+      static_assert(!CanBePiped<NotAView,    decltype(std::views::common)>);
+    }
+  }
+
+  {
+    static_assert(std::same_as<decltype(std::views::common), decltype(std::ranges::views::common)>);
+  }
+
+  return true;
+}
+
+int main(int, char**) {
+  test();
+  static_assert(test());
+
+  return 0;
+}

libcxx/test/std/ranges/range.adaptors/range.common.view/types.h

 //===----------------------------------------------------------------------===//
 //
 // 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 TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_COMMON_VIEW_TYPES_H
 #define TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_COMMON_VIEW_TYPES_H
 
+#include <ranges>
+
 #include "test_iterators.h"
 
 struct DefaultConstructibleView : std::ranges::view_base {
   int* begin_ = nullptr;
   int* end_ = nullptr;
 
   DefaultConstructibleView() = default;
   friend constexpr int* begin(DefaultConstructibleView& view) { return view.begin_; }

 // Required to make SizedRandomAccessView a sized view.
 constexpr auto operator-(sentinel_wrapper<RandomAccessIter> sent, RandomAccessIter iter) {
   return sent.base().base() - iter.base();
 }
 constexpr auto operator-(RandomAccessIter iter, sentinel_wrapper<RandomAccessIter> sent) {
   return iter.base() - sent.base().base();
 }
 
+struct CommonView : std::ranges::view_base {
+  int* begin_;
+  int* end_;
+  constexpr explicit CommonView(int* b, int* e) : begin_(b), end_(e) { }
+  friend constexpr int* begin(CommonView& view) { return view.begin_; }
+  friend constexpr int* begin(CommonView const& view) { return view.begin_; }
+  friend constexpr int* end(CommonView& view) { return view.end_; }
+  friend constexpr int* end(CommonView const& view) { return view.end_; }
+};
+static_assert(std::ranges::range<CommonView>);
+static_assert(std::ranges::common_range<CommonView>);
+
+struct NonCommonView : std::ranges::view_base {
+  int* begin_;
+  int* end_;
+  constexpr explicit NonCommonView(int* b, int* e) : begin_(b), end_(e) { }
+  friend constexpr int* begin(NonCommonView& view) { return view.begin_; }
+  friend constexpr int* begin(NonCommonView const& view) { return view.begin_; }
+  friend constexpr sentinel_wrapper<int*> end(NonCommonView& view) { return sentinel_wrapper<int*>(view.end_); }
+  friend constexpr sentinel_wrapper<int*> end(NonCommonView const& view) { return sentinel_wrapper<int*>(view.end_); }
+};
+static_assert( std::ranges::range<NonCommonView>);
+static_assert(!std::ranges::common_range<NonCommonView>);
+
 #endif // TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_COMMON_VIEW_TYPES_H