[libcxx][NFC] Consolidate testing concept CanBePiped #80154

hawkinsw · 2024-01-31T15:55:37Z

Almost every test needed a CanBePiped concept and each implemented it separately, but identically. Consolidate all implementations into test_range.h.

hawkinsw · 2024-01-31T15:56:01Z

I hope that this consolidation is helpful!

llvmbot · 2024-01-31T15:56:14Z

@llvm/pr-subscribers-libcxx

Author: Will Hawkins (hawkinsw)

Changes

Almost every test needed a CanBePiped concept and each implemented it separately, but identically. Consolidate all implementations into test_range.h.

Full diff: https://github.com/llvm/llvm-project/pull/80154.diff

15 Files Affected:

(modified) libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp (+2-6)
(modified) libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp (+1-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp (+1-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.drop.while/adaptor.pass.cpp (+2-6)
(modified) libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp (+2-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.elements/adaptor.pass.cpp (+2-6)
(modified) libcxx/test/std/ranges/range.adaptors/range.filter/adaptor.pass.cpp (+1-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.join/adaptor.pass.cpp (-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp (+1-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp (+1-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp (+1-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp (-6)
(modified) libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp (+2-5)
(modified) libcxx/test/std/ranges/range.adaptors/range.transform/adaptor.pass.cpp (-5)
(modified) libcxx/test/support/test_range.h (+5)

diff --git a/libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp
index d90a25d93e345..d31373fcad78b 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.all/all.pass.cpp
@@ -17,8 +17,9 @@
 #include <type_traits>
 #include <utility>
 
-#include "test_macros.h"
 #include "test_iterators.h"
+#include "test_macros.h"
+#include "test_range.h"
 
 int globalBuff[8];
 
@@ -82,11 +83,6 @@ struct RandomAccessRange {
 template<>
 inline constexpr bool std::ranges::enable_borrowed_range<RandomAccessRange> = true;
 
-template <class View, class T>
-concept CanBePiped = requires (View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
-
 constexpr bool test() {
   {
     ASSERT_SAME_TYPE(decltype(std::views::all(View<true>())), View<true>);
diff --git a/libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp
index 423c481127e20..697085a1d5c55 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.chunk.by/adaptor.pass.cpp
@@ -22,11 +22,7 @@
 #include <utility>
 
 #include "test_iterators.h"
-
-template <class View, class T>
-concept CanBePiped = requires(View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
+#include "test_range.h"
 
 struct Pred {
   constexpr bool operator()(int x, int y) const { return x != -y; }
diff --git a/libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp
index 143f7df1d3724..b0e72e9e79148 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.common.view/adaptor.pass.cpp
@@ -18,13 +18,9 @@
 #include <utility>
 
 #include "test_iterators.h"
+#include "test_range.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};
 
diff --git a/libcxx/test/std/ranges/range.adaptors/range.drop.while/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.drop.while/adaptor.pass.cpp
index 409b400f7f87f..c41d4172f1990 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.drop.while/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.drop.while/adaptor.pass.cpp
@@ -16,6 +16,8 @@
 #include <type_traits>
 #include <utility>
 
+#include "test_range.h"
+
 struct Pred {
   constexpr bool operator()(int i) const { return i < 3; }
 };
@@ -51,12 +53,6 @@ static_assert(std::is_invocable_v<decltype((std::views::drop_while)), int (&)[2]
 static_assert(!std::is_invocable_v<decltype((std::views::drop_while)), Foo (&)[2], Pred>);
 static_assert(std::is_invocable_v<decltype((std::views::drop_while)), MoveOnlyView, Pred>);
 
-template <class View, class T>
-concept CanBePiped =
-    requires(View&& view, T&& t) {
-      { std::forward<View>(view) | std::forward<T>(t) };
-    };
-
 static_assert(!CanBePiped<MoveOnlyView, decltype(std::views::drop_while)>);
 static_assert(CanBePiped<MoveOnlyView, decltype(std::views::drop_while(Pred{}))>);
 static_assert(!CanBePiped<int, decltype(std::views::drop_while(Pred{}))>);
diff --git a/libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp
index 457b137c80457..d07fe9ac0ea72 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp
@@ -18,12 +18,9 @@
 #include <span>
 #include <string_view>
 #include <utility>
-#include "test_iterators.h"
 
-template <class View, class T>
-concept CanBePiped = requires (View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
+#include "test_iterators.h"
+#include "test_range.h"
 
 struct SizedView : std::ranges::view_base {
   int* begin_ = nullptr;
diff --git a/libcxx/test/std/ranges/range.adaptors/range.elements/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.elements/adaptor.pass.cpp
index d68d6e57e2ed5..35589c96e4b77 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.elements/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.elements/adaptor.pass.cpp
@@ -19,6 +19,8 @@
 #include <type_traits>
 #include <utility>
 
+#include "test_range.h"
+
 template <class T>
 struct View : std::ranges::view_base {
   T* begin() const;
@@ -41,12 +43,6 @@ static_assert(!std::is_invocable_v<decltype((std::views::values)), View<int>>);
 static_assert(std::is_invocable_v<decltype((std::views::values)), View<std::pair<int, int>>>);
 static_assert(!std::is_invocable_v<decltype((std::views::values)), View<std::tuple<int>>>);
 
-template <class View, class T>
-concept CanBePiped =
-    requires(View&& view, T&& t) {
-      { std::forward<View>(view) | std::forward<T>(t) };
-    };
-
 static_assert(!CanBePiped<View<int>, decltype((std::views::elements<0>))>);
 static_assert(CanBePiped<View<std::pair<int, int>>, decltype((std::views::elements<0>))>);
 static_assert(CanBePiped<View<std::tuple<int>>, decltype((std::views::elements<0>))>);
diff --git a/libcxx/test/std/ranges/range.adaptors/range.filter/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.filter/adaptor.pass.cpp
index 1adf1b4630b87..a44e96e1d4bde 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.filter/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.filter/adaptor.pass.cpp
@@ -19,11 +19,7 @@
 #include <utility>
 
 #include "test_iterators.h"
-
-template <class View, class T>
-concept CanBePiped = requires (View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
+#include "test_range.h"
 
 struct NonCopyablePredicate {
   NonCopyablePredicate(NonCopyablePredicate const&) = delete;
diff --git a/libcxx/test/std/ranges/range.adaptors/range.join/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.join/adaptor.pass.cpp
index 9beb3d282a27c..54d95edf510f4 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.join/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.join/adaptor.pass.cpp
@@ -32,11 +32,6 @@ struct Foo {
   constexpr Foo(int ii) : i(ii) {}
 };
 
-template <class View, class T>
-concept CanBePiped = requires(View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
-
 constexpr bool test() {
   int buffer1[3] = {1, 2, 3};
   int buffer2[2] = {4, 5};
diff --git a/libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp
index 6bfa0ab487ba1..7bb7e95b02dab 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.lazy.split/adaptor.pass.cpp
@@ -19,13 +19,9 @@
 #include <utility>
 
 #include "test_iterators.h"
+#include "test_range.h"
 #include "types.h"
 
-template <class View, class T>
-concept CanBePiped = requires (View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
-
 struct SomeView : std::ranges::view_base {
   const std::string_view* v_;
   constexpr SomeView(const std::string_view& v) : v_(&v) {}
diff --git a/libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp
index 5e0a0a3ce3fbc..26c9ac08d338b 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.reverse/adaptor.pass.cpp
@@ -17,13 +17,9 @@
 #include <iterator>
 #include <utility>
 
+#include "test_range.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};
 
diff --git a/libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp
index 85d13ac5c29df..34c0f15ce3463 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.split/adaptor.pass.cpp
@@ -19,11 +19,7 @@
 #include <utility>
 
 #include "test_iterators.h"
-
-template <class View, class T>
-concept CanBePiped = requires (View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
+#include "test_range.h"
 
 struct SomeView : std::ranges::view_base {
   const std::string_view* v_;
diff --git a/libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp
index 8796f9df63cee..42814014ce884 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp
@@ -42,12 +42,6 @@ static_assert(std::is_invocable_v<decltype((std::views::take_while)), int (&)[2]
 static_assert(!std::is_invocable_v<decltype((std::views::take_while)), Foo (&)[2], Pred>);
 static_assert(std::is_invocable_v<decltype((std::views::take_while)), MoveOnlyView, Pred>);
 
-template <class View, class T>
-concept CanBePiped =
-    requires(View&& view, T&& t) {
-      { std::forward<View>(view) | std::forward<T>(t) };
-    };
-
 static_assert(!CanBePiped<MoveOnlyView, decltype(std::views::take_while)>);
 static_assert(CanBePiped<MoveOnlyView, decltype(std::views::take_while(Pred{}))>);
 static_assert(!CanBePiped<int, decltype(std::views::take_while(Pred{}))>);
diff --git a/libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp
index bb5b5f5ff4909..b30c4fbe3fd96 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.take/adaptor.pass.cpp
@@ -17,12 +17,9 @@
 #include <span>
 #include <string_view>
 #include <utility>
-#include "test_iterators.h"
 
-template <class View, class T>
-concept CanBePiped = requires (View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
+#include "test_iterators.h"
+#include "test_range.h"
 
 struct SizedView : std::ranges::view_base {
   int* begin_ = nullptr;
diff --git a/libcxx/test/std/ranges/range.adaptors/range.transform/adaptor.pass.cpp b/libcxx/test/std/ranges/range.adaptors/range.transform/adaptor.pass.cpp
index 59dd85bf28bc3..91eedc5d36a96 100644
--- a/libcxx/test/std/ranges/range.adaptors/range.transform/adaptor.pass.cpp
+++ b/libcxx/test/std/ranges/range.adaptors/range.transform/adaptor.pass.cpp
@@ -20,11 +20,6 @@
 #include "test_macros.h"
 #include "types.h"
 
-template <class View, class T>
-concept CanBePiped = requires (View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
-
 struct NonCopyableFunction {
   NonCopyableFunction(NonCopyableFunction const&) = delete;
   template <class T>
diff --git a/libcxx/test/support/test_range.h b/libcxx/test/support/test_range.h
index 6061f710a2636..c5eeb25bb59eb 100644
--- a/libcxx/test/support/test_range.h
+++ b/libcxx/test/support/test_range.h
@@ -89,4 +89,9 @@ concept simple_view =
     std::same_as<std::ranges::iterator_t<Range>, std::ranges::iterator_t<const Range>> &&
     std::same_as<std::ranges::sentinel_t<Range>, std::ranges::sentinel_t<const Range>>;
 
+template <class View, class T>
+concept CanBePiped = requires(View&& view, T&& t) {
+  { std::forward<View>(view) | std::forward<T>(t) };
+};
+
 #endif // LIBCXX_TEST_SUPPORT_TEST_RANGE_H

hawkinsw · 2024-02-06T02:07:29Z

@cjdb I hope that this is something that is helpful!

hawkinsw · 2024-02-10T01:29:42Z

Hello! Let me know if there is anything else I can do to get this into shape! cc @cjdb @var-const

var-const

Thanks, this is a nice refactoring! Just wanted to double-check whether a couple of these files need to include test_range.h now (or perhaps they aren't actually using CanBePiped?). Other than that, LGTM.

var-const · 2024-02-13T01:23:08Z

libcxx/test/std/ranges/range.adaptors/range.transform/adaptor.pass.cpp

@@ -20,11 +20,6 @@
 #include "test_macros.h"


Do you need to include "test_range.h" here? Otherwise, this file is probably relying on a transitive include (unless CanBePiped is not used here?).

Great eyes! In these cases, I was relying on the fact that the implementers of the adapters created their own header files (types.h) and those file includes the test_range.h. I have adjusted the patch to make direct inclusion of the test_ranges.h file but I am more than happy to go back to the old version if that is our standard.

Thanks for the sharp feedback!

var-const · 2024-02-13T01:23:29Z

libcxx/test/std/ranges/range.adaptors/range.take.while/adaptor.pass.cpp

-    requires(View&& view, T&& t) {
-      { std::forward<View>(view) | std::forward<T>(t) };
-    };
-
 static_assert(!CanBePiped<MoveOnlyView, decltype(std::views::take_while)>);


Include "test_range.h"? (or is it not used in this file?)

var-const · 2024-02-13T01:24:00Z

libcxx/test/std/ranges/range.adaptors/range.join/adaptor.pass.cpp

-concept CanBePiped = requires(View&& view, T&& t) {
-  { std::forward<View>(view) | std::forward<T>(t) };
-};
-
 constexpr bool test() {


Include "test_range.h"? (or is it not used in this file?)

Almost every test needed a CanBePiped concept and each implemented it separately, but identically. Consolidate all implementations into test_range.h.

Directly import test_range.h in several places.

hawkinsw · 2024-02-27T18:51:07Z

Just wanted to see if my replies addressed your comments! Thanks for everything!
Will

hawkinsw · 2024-03-05T17:41:09Z

Just wanted to see if my replies addressed your comments! Thanks for everything! Will

Please let me know if there is anything else I can do to clean this up!

var-const

Sorry about the delay! LGTM.

hawkinsw requested review from var-const and cjdb January 31, 2024 15:55

hawkinsw self-assigned this Jan 31, 2024

hawkinsw requested a review from a team as a code owner January 31, 2024 15:55

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jan 31, 2024

hawkinsw force-pushed the can_be_piped branch from 21d1694 to 68ee310 Compare January 31, 2024 22:00

var-const reviewed Feb 13, 2024

View reviewed changes

hawkinsw added 2 commits February 13, 2024 01:52

[libcxx][NFC] Consolidate testing concept CanBePiped

73c1313

Almost every test needed a CanBePiped concept and each implemented it separately, but identically. Consolidate all implementations into test_range.h.

fixup! [libcxx][NFC] Consolidate testing concept CanBePiped

d844cf1

Directly import test_range.h in several places.

hawkinsw force-pushed the can_be_piped branch from 68ee310 to d844cf1 Compare February 13, 2024 07:18

hawkinsw requested a review from var-const February 13, 2024 07:18

var-const self-assigned this Mar 8, 2024

var-const added the ranges Issues related to `<ranges>` label Mar 8, 2024

var-const approved these changes Mar 8, 2024

View reviewed changes

ldionne merged commit ba2236d into llvm:main Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[libcxx][NFC] Consolidate testing concept CanBePiped #80154

[libcxx][NFC] Consolidate testing concept CanBePiped #80154

Uh oh!

hawkinsw commented Jan 31, 2024

Uh oh!

hawkinsw commented Jan 31, 2024

Uh oh!

llvmbot commented Jan 31, 2024

Uh oh!

hawkinsw commented Feb 6, 2024

Uh oh!

hawkinsw commented Feb 10, 2024

Uh oh!

var-const left a comment

Uh oh!

var-const Feb 13, 2024

Uh oh!

hawkinsw Feb 13, 2024

Uh oh!

var-const Feb 13, 2024

Uh oh!

var-const Feb 13, 2024

Uh oh!

hawkinsw commented Feb 27, 2024

Uh oh!

hawkinsw commented Mar 5, 2024

Uh oh!

var-const left a comment

Uh oh!

Uh oh!

[libcxx][NFC] Consolidate testing concept CanBePiped #80154

[libcxx][NFC] Consolidate testing concept CanBePiped #80154

Uh oh!

Conversation

hawkinsw commented Jan 31, 2024

Uh oh!

hawkinsw commented Jan 31, 2024

Uh oh!

llvmbot commented Jan 31, 2024

Uh oh!

hawkinsw commented Feb 6, 2024

Uh oh!

hawkinsw commented Feb 10, 2024

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

var-const Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

hawkinsw Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

var-const Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

var-const Feb 13, 2024

Choose a reason for hiding this comment

Uh oh!

hawkinsw commented Feb 27, 2024

Uh oh!

hawkinsw commented Mar 5, 2024

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!