Skip to content

[libcxx] applies changes regarding post-commit feedback to #75259 #76534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

cjdb
Copy link
Contributor

@cjdb cjdb commented Dec 28, 2023

Some of the feedback was also relevant to other files, and has been applied there too.

Some of the feedback was also relevant to other files, and has been
applied there too.
@cjdb cjdb added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Dec 28, 2023
@cjdb cjdb requested a review from a team as a code owner December 28, 2023 21:17
@llvmbot
Copy link
Member

llvmbot commented Dec 28, 2023

@llvm/pr-subscribers-libcxx

Author: Christopher Di Bella (cjdb)

Changes

Some of the feedback was also relevant to other files, and has been applied there too.


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

10 Files Affected:

  • (modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/left_folds.pass.cpp (+15-6)
  • (modified) libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/requirements.compile.pass.cpp (+15-2)
  • (modified) libcxx/test/std/algorithms/algorithms.results/in_found_result.pass.cpp (+9-3)
  • (modified) libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp (+6-1)
  • (modified) libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp (+5)
  • (modified) libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp (+5)
  • (modified) libcxx/test/std/algorithms/algorithms.results/in_out_out_result.pass.cpp (+5)
  • (modified) libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp (+5)
  • (modified) libcxx/test/std/algorithms/algorithms.results/in_value_result.pass.cpp (+6-1)
  • (modified) libcxx/test/std/algorithms/algorithms.results/min_max_result.pass.cpp (+5)
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/left_folds.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/left_folds.pass.cpp
index cf089b27c76e02..ebe00c6b8c258d 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/left_folds.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/left_folds.pass.cpp
@@ -56,14 +56,14 @@ concept is_in_value_result =
 template <class Result, class T>
 concept is_dangling_with = std::same_as<Result, std::ranges::fold_left_with_iter_result<std::ranges::dangling, T>>;
 
-struct Long {
+struct Integer {
   int value;
 
-  constexpr Long(int const x) : value(x) {}
+  constexpr Integer(int const x) : value(x) {}
 
-  constexpr Long plus(int const x) const { return Long{value + x}; }
+  constexpr Integer plus(int const x) const { return Integer{value + x}; }
 
-  friend constexpr bool operator==(Long const& x, Long const& y) = default;
+  friend constexpr bool operator==(Integer const& x, Integer const& y) = default;
 };
 
 template <std::ranges::input_range R, class T, class F, std::equality_comparable Expected>
@@ -74,6 +74,7 @@ constexpr void check_iterator(R& r, T const& init, F f, Expected const& expected
     assert(result.in == r.end());
     assert(result.value == expected);
   }
+
   {
     auto telemetry                                        = invocable_telemetry();
     auto f2                                               = invocable_with_telemetry(f, telemetry);
@@ -89,6 +90,7 @@ constexpr void check_iterator(R& r, T const& init, F f, Expected const& expected
     std::same_as<Expected> decltype(auto) result = fold_left(r.begin(), r.end(), init, f);
     assert(result == expected);
   }
+
   {
     auto telemetry                               = invocable_telemetry();
     auto f2                                      = invocable_with_telemetry(f, telemetry);
@@ -108,6 +110,7 @@ constexpr void check_lvalue_range(R& r, T const& init, F f, Expected const& expe
     assert(result.in == r.end());
     assert(result.value == expected);
   }
+
   {
     auto telemetry                               = invocable_telemetry();
     auto f2                                      = invocable_with_telemetry(f, telemetry);
@@ -122,6 +125,7 @@ constexpr void check_lvalue_range(R& r, T const& init, F f, Expected const& expe
     std::same_as<Expected> decltype(auto) result = fold_left(r, init, f);
     assert(result == expected);
   }
+
   {
     auto telemetry                               = invocable_telemetry();
     auto f2                                      = invocable_with_telemetry(f, telemetry);
@@ -141,6 +145,7 @@ constexpr void check_rvalue_range(R& r, T const& init, F f, Expected const& expe
     is_dangling_with<Expected> decltype(auto) result = fold_left_with_iter(std::move(r2), init, f);
     assert(result.value == expected);
   }
+
   {
     auto telemetry                                   = invocable_telemetry();
     auto f2                                          = invocable_with_telemetry(f, telemetry);
@@ -157,6 +162,7 @@ constexpr void check_rvalue_range(R& r, T const& init, F f, Expected const& expe
     std::same_as<Expected> decltype(auto) result = fold_left(std::move(r2), init, f);
     assert(result == expected);
   }
+
   {
     auto telemetry                               = invocable_telemetry();
     auto f2                                      = invocable_with_telemetry(f, telemetry);
@@ -183,7 +189,7 @@ constexpr void empty_range_test_case() {
   check(data, -100, std::multiplies(), -100);
 
   check(data | std::views::take_while([](auto) { return false; }), 1.23, std::plus(), 1.23);
-  check(data, Long(52), &Long::plus, Long(52));
+  check(data, Integer(52), &Integer::plus, Integer(52));
 }
 
 constexpr void common_range_test_case() {
@@ -206,7 +212,7 @@ constexpr void common_range_test_case() {
   };
   check(data, 0, fib, fibonacci(data.back()));
 
-  check(data, Long(0), &Long::plus, Long(triangular_sum(data)));
+  check(data, Integer(0), &Integer::plus, Integer(triangular_sum(data)));
 }
 
 constexpr void non_common_range_test_case() {
@@ -266,6 +272,7 @@ void runtime_only_test_case() {
       assert(result.in == data.end());
       assert(result.value == expected);
     }
+
     {
       auto input = std::istringstream(raw_data);
       auto data  = std::views::istream<std::string>(input);
@@ -274,11 +281,13 @@ void runtime_only_test_case() {
       assert(result.in == data.end());
       assert(result.value == expected);
     }
+
     {
       auto input = std::istringstream(raw_data);
       auto data  = std::views::istream<std::string>(input);
       assert(fold_left(data.begin(), data.end(), init, std::plus()) == expected);
     }
+
     {
       auto input = std::istringstream(raw_data);
       auto data  = std::views::istream<std::string>(input);
diff --git a/libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/requirements.compile.pass.cpp b/libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/requirements.compile.pass.cpp
index cad96d4c10127f..261dd6e572e507 100644
--- a/libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/requirements.compile.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.nonmodifying/alg.fold/requirements.compile.pass.cpp
@@ -10,8 +10,21 @@
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 
-// Checks that `std::ranges::fold_left_with_iter`'s requirements reject parameters that don't meet
-// the overloads' constraints.
+// template<input_iterator I, sentinel_for<I> S, class T,
+//          indirectly-binary-left-foldable<T, I> F>
+//   constexpr see below ranges::fold_left_with_iter(I first, S last, T init, F f);
+//
+// template<input_range R, class T, indirectly-binary-left-foldable<T, iterator_t<R>> F>
+//   constexpr see below ranges::fold_left_with_iter(R&& r, T init, F f);
+
+// template<input_iterator I, sentinel_for<I> S, class T,
+//          indirectly-binary-left-foldable<T, I> F>
+//   constexpr see below ranges::fold_left(I first, S last, T init, F f);
+//
+// template<input_range R, class T, indirectly-binary-left-foldable<T, iterator_t<R>> F>
+//   constexpr see below ranges::fold_left(R&& r, T init, F f);
+
+// Checks that the algorithm requirements reject parameters that don't meet the overloads' constraints.
 
 #include <algorithm>
 #include <concepts>
diff --git a/libcxx/test/std/algorithms/algorithms.results/in_found_result.pass.cpp b/libcxx/test/std/algorithms/algorithms.results/in_found_result.pass.cpp
index af23a1b051c89e..ec104df5feb47e 100644
--- a/libcxx/test/std/algorithms/algorithms.results/in_found_result.pass.cpp
+++ b/libcxx/test/std/algorithms/algorithms.results/in_found_result.pass.cpp
@@ -64,6 +64,7 @@ struct ConvertibleFrom {
 };
 
 constexpr bool test() {
+  // Checks that conversion operations are correct.
   {
     std::ranges::in_found_result<double> res{10, true};
     assert(res.in == 10);
@@ -72,6 +73,8 @@ constexpr bool test() {
     assert(res2.in.content == 10);
     assert(res2.found == true);
   }
+
+  // Checks that conversions are possible when one of the types is move-only.
   {
     std::ranges::in_found_result<MoveOnly> res{MoveOnly{}, false};
     assert(res.in.get() == 1);
@@ -82,10 +85,13 @@ constexpr bool test() {
     assert(res.in.get() == 0);
     assert(!res.found);
   }
-  auto [in, found] = std::ranges::in_found_result<int>{2, false};
-  assert(in == 2);
-  assert(!found);
 
+  // Checks that structured bindings get the correct values.
+  {
+    auto [in, found] = std::ranges::in_found_result<int>{2, false};
+    assert(in == 2);
+    assert(!found);
+  }
   return true;
 }
 
diff --git a/libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp b/libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp
index f44b5988d849ad..4395380ed179aa 100644
--- a/libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp
+++ b/libcxx/test/std/algorithms/algorithms.results/in_fun_result.pass.cpp
@@ -48,7 +48,7 @@ static_assert(std::is_convertible_v<const std::ranges::in_fun_result<int, int>&&
 static_assert(std::is_move_constructible_v<std::ranges::in_fun_result<MoveOnly, int>>);
 static_assert(std::is_move_constructible_v<std::ranges::in_fun_result<int, MoveOnly>>);
 
-// should not copy constructible with move-only type
+// should not be copy constructible with move-only type
 static_assert(!std::is_copy_constructible_v<std::ranges::in_fun_result<MoveOnly, int>>);
 static_assert(!std::is_copy_constructible_v<std::ranges::in_fun_result<int, MoveOnly>>);
 
@@ -64,6 +64,7 @@ struct ConvertibleFrom {
 };
 
 constexpr bool test() {
+  // Checks that conversion operations are correct.
   {
     std::ranges::in_fun_result<int, double> res{10, 0.};
     assert(res.in == 10);
@@ -72,6 +73,8 @@ constexpr bool test() {
     assert(res2.in.content == 10);
     assert(res2.fun.content == 0.);
   }
+
+  // Checks that conversions are possible when one of the types is move-only.
   {
     std::ranges::in_fun_result<MoveOnly, int> res{MoveOnly{}, 2};
     assert(res.in.get() == 1);
@@ -81,6 +84,8 @@ constexpr bool test() {
     assert(res2.in.get() == 1);
     assert(res2.fun == 2);
   }
+
+  // Checks that structured bindings get the correct values.
   {
     auto [in, fun] = std::ranges::in_fun_result<int, int>{1, 2};
     assert(in == 1);
diff --git a/libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp b/libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp
index c9ab2d430b5002..0ee09ceb474167 100644
--- a/libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp
+++ b/libcxx/test/std/algorithms/algorithms.results/in_in_out_result.pass.cpp
@@ -67,6 +67,7 @@ struct ConvertibleFrom {
 };
 
 constexpr bool test() {
+  // Checks that conversion operations are correct.
   {
     std::ranges::in_in_out_result<int, double, float> res{10, 0., 1.f};
     assert(res.in1 == 10);
@@ -77,6 +78,8 @@ constexpr bool test() {
     assert(res2.in2.content == 0.);
     assert(res2.out.content == 1.f);
   }
+
+  // Checks that conversions are possible when one of the types is move-only.
   {
     std::ranges::in_in_out_result<MoveOnly, int, int> res1{MoveOnly{}, 0, 0};
     assert(res1.in1.get() == 1);
@@ -84,6 +87,8 @@ constexpr bool test() {
     assert(res1.in1.get() == 0);
     assert(res2.in1.get() == 1);
   }
+
+  // Checks that structured bindings get the correct values.
   {
     auto [in1, in2, out] = std::ranges::in_in_out_result<int, int, int>{1, 2, 3};
     assert(in1 == 1);
diff --git a/libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp b/libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
index 64d1106117329c..10e6e88327fe18 100644
--- a/libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
+++ b/libcxx/test/std/algorithms/algorithms.results/in_in_result.pass.cpp
@@ -59,6 +59,7 @@ struct ConstructibleFrom {
 };
 
 constexpr bool test() {
+  // Checks that conversion operations are correct.
   {
     std::ranges::in_in_result<int, double> res{10L, 0.};
     assert(res.in1 == 10);
@@ -67,12 +68,16 @@ constexpr bool test() {
     assert(res2.in1.content == 10);
     assert(res2.in2.content == 0.);
   }
+
+  // Checks that conversions are possible when one of the types is move-only.
   {
     std::ranges::in_in_result<MoveOnly, int> res{MoveOnly{}, 0};
     assert(res.in1.get() == 1);
     [[maybe_unused]] auto res2 = static_cast<std::ranges::in_in_result<MoveOnly, int>>(std::move(res));
     assert(res.in1.get() == 0);
   }
+
+  // Checks that structured bindings get the correct values.
   {
     auto [in1, in2] = std::ranges::in_in_result<int, int>{1, 2};
     assert(in1 == 1);
diff --git a/libcxx/test/std/algorithms/algorithms.results/in_out_out_result.pass.cpp b/libcxx/test/std/algorithms/algorithms.results/in_out_out_result.pass.cpp
index 682462bc9a5ba1..875080b1900e05 100644
--- a/libcxx/test/std/algorithms/algorithms.results/in_out_out_result.pass.cpp
+++ b/libcxx/test/std/algorithms/algorithms.results/in_out_out_result.pass.cpp
@@ -65,6 +65,7 @@ struct ConvertibleFrom {
 };
 
 constexpr bool test() {
+  // Checks that conversion operations are correct.
   {
     std::ranges::in_out_out_result<int, double, float> res{10, 0., 1.f};
     assert(res.in == 10);
@@ -75,6 +76,8 @@ constexpr bool test() {
     assert(res2.out1.content == 0.);
     assert(res2.out2.content == 1.f);
   }
+
+  // Checks that conversions are possible when one of the types is move-only.
   {
     std::ranges::in_out_out_result<MoveOnly, int, int> res1{MoveOnly{}, 0, 0};
     assert(res1.in.get() == 1);
@@ -82,6 +85,8 @@ constexpr bool test() {
     assert(res1.in.get() == 0);
     assert(res2.in.get() == 1);
   }
+
+  // Checks that structured bindings get the correct values.
   {
     auto [in, out1, out2] = std::ranges::in_out_out_result<int, int, int>{1, 2, 3};
     assert(in == 1);
diff --git a/libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp b/libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp
index ca9f4464dee5fb..9cfb5f43955cb0 100644
--- a/libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp
+++ b/libcxx/test/std/algorithms/algorithms.results/in_out_result.pass.cpp
@@ -59,6 +59,7 @@ struct ConvertibleFrom {
 };
 
 constexpr bool test() {
+  // Checks that conversion operations are correct.
   {
     std::ranges::in_out_result<double, int> res{10, 1};
     assert(res.in == 10);
@@ -67,6 +68,8 @@ constexpr bool test() {
     assert(res2.in.content == 10);
     assert(res2.out.content == 1);
   }
+
+  // Checks that conversions are possible when one of the types is move-only.
   {
     std::ranges::in_out_result<MoveOnly, int> res{MoveOnly{}, 10};
     assert(res.in.get() == 1);
@@ -77,6 +80,8 @@ constexpr bool test() {
     assert(res2.in.get() == 1);
     assert(res2.out == 10);
   }
+
+  // Checks that structured bindings get the correct values.
   {
     auto [min, max] = std::ranges::in_out_result<int, int>{1, 2};
     assert(min == 1);
diff --git a/libcxx/test/std/algorithms/algorithms.results/in_value_result.pass.cpp b/libcxx/test/std/algorithms/algorithms.results/in_value_result.pass.cpp
index f4023b89dce703..b7aba0c88ee950 100644
--- a/libcxx/test/std/algorithms/algorithms.results/in_value_result.pass.cpp
+++ b/libcxx/test/std/algorithms/algorithms.results/in_value_result.pass.cpp
@@ -53,7 +53,7 @@ static_assert(
 static_assert(std::is_move_constructible_v<std::ranges::in_value_result<MoveOnly, int>>);
 static_assert(std::is_move_constructible_v<std::ranges::in_value_result<int, MoveOnly>>);
 
-// should not copy constructible with move-only type
+// should not be copy constructible with move-only type
 static_assert(!std::is_copy_constructible_v<std::ranges::in_value_result<MoveOnly, int>>);
 static_assert(!std::is_copy_constructible_v<std::ranges::in_value_result<int, MoveOnly>>);
 
@@ -71,6 +71,7 @@ struct ConvertibleFrom {
 };
 
 constexpr bool test() {
+  // Checks that conversion operations are correct.
   {
     std::ranges::in_value_result<int, double> res{10, 0.};
     assert(res.in == 10);
@@ -79,6 +80,8 @@ constexpr bool test() {
     assert(res2.in.content == 10);
     assert(res2.value.content == 0.);
   }
+
+  // Checks that conversions are possible when one of the types is move-only.
   {
     std::ranges::in_value_result<MoveOnly, int> res{MoveOnly{}, 2};
     assert(res.in.get() == 1);
@@ -88,6 +91,8 @@ constexpr bool test() {
     assert(res2.in.get() == 1);
     assert(res2.value == 2);
   }
+
+  // Checks that structured bindings get the correct values.
   {
     auto [in, value] = std::ranges::in_value_result<int, int>{1, 2};
     assert(in == 1);
diff --git a/libcxx/test/std/algorithms/algorithms.results/min_max_result.pass.cpp b/libcxx/test/std/algorithms/algorithms.results/min_max_result.pass.cpp
index 23c22ec9d3924a..bc394926c9548b 100644
--- a/libcxx/test/std/algorithms/algorithms.results/min_max_result.pass.cpp
+++ b/libcxx/test/std/algorithms/algorithms.results/min_max_result.pass.cpp
@@ -60,6 +60,7 @@ struct ConvertibleFrom {
 };
 
 constexpr bool test() {
+  // Checks that conversion operations are correct.
   {
     std::ranges::min_max_result<double> res{10, 1};
     assert(res.min == 10);
@@ -68,6 +69,8 @@ constexpr bool test() {
     assert(res2.min.content == 10);
     assert(res2.max.content == 1);
   }
+
+  // Checks that conversions are possible when one of the types is move-only.
   {
     std::ranges::min_max_result<MoveOnly> res{MoveOnly{}, MoveOnly{}};
     assert(res.min.get() == 1);
@@ -78,6 +81,8 @@ constexpr bool test() {
     assert(res2.min.get() == 1);
     assert(res2.max.get() == 1);
   }
+
+  // Checks that structured bindings get the correct values.
   {
     auto [min, max] = std::ranges::min_max_result<int>{1, 2};
     assert(min == 1);

@cjdb
Copy link
Contributor Author

cjdb commented Apr 11, 2024

The failing test is unrelated to the PR.

@cjdb cjdb merged commit f0ea888 into llvm:main Apr 11, 2024
@cjdb cjdb deleted the feedback-75259 branch April 11, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants