Skip to content
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

[libc++][NFC] Improve test readability for std::fill_n #133771

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

winner245
Copy link
Contributor

@winner245 winner245 commented Mar 31, 2025

This patch enhances test readability by inlining standalone tests, eliminating unnecessary navigation. Additionally, several classes with ad-hoc names have been renamed for better clarity:

  • A -> CharWrapper as it wraps a char
  • B -> CharTransformer as it accepts a char xc but stores xc + 1
  • Storage -> CharUnionStorage as it stores a union of 2 chars.

This patch addresses a follow-up comment from PR #120909 to inline tests.

@winner245 winner245 force-pushed the inline-fill_n-tests branch from 29dfa88 to 45e2fbc Compare March 31, 2025 19:04
@winner245 winner245 force-pushed the inline-fill_n-tests branch from 45e2fbc to 46b1333 Compare April 3, 2025 02:40
@winner245 winner245 marked this pull request as ready for review April 8, 2025 21:38
@winner245 winner245 requested a review from a team as a code owner April 8, 2025 21:38
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Apr 8, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 8, 2025

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

This patch enhances test readability by inlining standalone tests, eliminating unnecessary navigation. Additionally, several classes with ad-hoc names have been renamed for better clarity:

  • A -> CharWrapper as it wraps a char
  • B -> CharTransformer as it accepts a char xc but stores xc + 1
  • Storage -> CharUnionStorage as it stores a union of 2 chars.

This patch addresses a follow-up comment from PR #120909 to inline tests.


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

1 Files Affected:

  • (modified) libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp (+57-61)
diff --git a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp
index 4dda8714d2cfa..e42172ceb960d 100644
--- a/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp
+++ b/libcxx/test/std/algorithms/alg.modifying.operations/alg.fill/fill_n.pass.cpp
@@ -52,57 +52,29 @@ struct Test {
   }
 };
 
-TEST_CONSTEXPR_CXX20 void test_int_array() {
-  {
-    int a[4] = {};
-    assert(std::fill_n(a, UDI(4), static_cast<char>(1)) == a + 4);
-    assert(a[0] == 1 && a[1] == 1 && a[2] == 1 && a[3] == 1);
-  }
-#if TEST_STD_VER >= 11
-  {
-    const std::size_t N = 5;
-    int ib[]            = {0, 0, 0, 0, 0, 0}; // one bigger than N
-
-    auto it = std::fill_n(std::begin(ib), N, 5);
-    assert(it == (std::begin(ib) + N) && std::all_of(std::begin(ib), it, [](int a) { return a == 5; }) &&
-           *it == 0 // don't overwrite the last value in the output array
-    );
-  }
-#endif
-}
-
 struct source {
   TEST_CONSTEXPR source() = default;
   TEST_CONSTEXPR_CXX20 operator int() const { return 1; }
 };
 
-TEST_CONSTEXPR_CXX20 void test_int_array_struct_source() {
-  int a[4] = {};
-  assert(std::fill_n(a, UDI(4), source()) == a + 4);
-  assert(a[0] == 1);
-  assert(a[1] == 1);
-  assert(a[2] == 1);
-  assert(a[3] == 1);
-}
-
-class A {
+class CharWrapper {
   char a_;
 
 public:
-  TEST_CONSTEXPR A() : a_('a') {};
-  TEST_CONSTEXPR explicit A(char a) : a_(a) {}
+  TEST_CONSTEXPR CharWrapper() : a_('a') {};
+  TEST_CONSTEXPR explicit CharWrapper(char a) : a_(a) {}
   TEST_CONSTEXPR operator unsigned char() const { return 'b'; }
 
-  TEST_CONSTEXPR friend bool operator==(const A& x, const A& y) { return x.a_ == y.a_; }
+  TEST_CONSTEXPR friend bool operator==(const CharWrapper& x, const CharWrapper& y) { return x.a_ == y.a_; }
 };
 
-struct B {
-  TEST_CONSTEXPR B() : c(0) {}
-  TEST_CONSTEXPR B(char xc) : c(xc + 1) {}
+struct CharTransformer {
+  TEST_CONSTEXPR CharTransformer() : c(0) {}
+  TEST_CONSTEXPR CharTransformer(char xc) : c(xc + 1) {}
   char c;
 };
 
-struct Storage {
+struct CharUnionStorage {
   union {
     unsigned char a;
     unsigned char b;
@@ -154,35 +126,59 @@ TEST_CONSTEXPR_CXX20 bool test_vector_bool(std::size_t N) {
   return true;
 }
 
-TEST_CONSTEXPR_CXX20 void test_struct_array() {
-  {
-    A a[3];
-    assert(std::fill_n(&a[0], UDI(3), A('a')) == a + 3);
-    assert(a[0] == A('a'));
-    assert(a[1] == A('a'));
-    assert(a[2] == A('a'));
-  }
-  {
-    B b[4] = {};
-    assert(std::fill_n(b, UDI(4), static_cast<char>(10)) == b + 4);
-    assert(b[0].c == 11);
-    assert(b[1].c == 11);
-    assert(b[2].c == 11);
-    assert(b[3].c == 11);
-  }
-  {
-    Storage foo[5];
-    std::fill_n(&foo[0], UDI(5), Storage());
-  }
-}
-
 TEST_CONSTEXPR_CXX20 bool test() {
   types::for_each(types::forward_iterator_list<char*>(), Test<char>());
   types::for_each(types::forward_iterator_list<int*>(), Test<int>());
 
-  test_int_array();
-  test_struct_array();
-  test_int_array_struct_source();
+  { // Test with int arrays
+    {
+      int a[4] = {};
+      assert(std::fill_n(a, UDI(4), static_cast<char>(1)) == a + 4);
+      assert(a[0] == 1 && a[1] == 1 && a[2] == 1 && a[3] == 1);
+    }
+#if TEST_STD_VER >= 11
+    {
+      const std::size_t N = 5;
+      int ib[]            = {0, 0, 0, 0, 0, 0}; // one bigger than N
+
+      auto it = std::fill_n(std::begin(ib), N, 5);
+      assert(it == (std::begin(ib) + N) && std::all_of(std::begin(ib), it, [](int a) { return a == 5; }) &&
+             *it == 0 // don't overwrite the last value in the output array
+      );
+    }
+#endif
+  }
+
+  { // Test with struct arrays
+    {
+      CharWrapper a[3];
+      assert(std::fill_n(&a[0], UDI(3), CharWrapper('a')) == a + 3);
+      assert(a[0] == CharWrapper('a'));
+      assert(a[1] == CharWrapper('a'));
+      assert(a[2] == CharWrapper('a'));
+    }
+    {
+      CharTransformer b[4] = {};
+      assert(std::fill_n(b, UDI(4), static_cast<char>(10)) == b + 4);
+      assert(b[0].c == 11);
+      assert(b[1].c == 11);
+      assert(b[2].c == 11);
+      assert(b[3].c == 11);
+    }
+    {
+      CharUnionStorage foo[5];
+      std::fill_n(&foo[0], UDI(5), CharUnionStorage());
+    }
+  }
+
+  { // Test with an int array and struct source
+    int a[4] = {};
+    assert(std::fill_n(a, UDI(4), source()) == a + 4);
+    assert(a[0] == 1);
+    assert(a[1] == 1);
+    assert(a[2] == 1);
+    assert(a[3] == 1);
+  }
 
   { // Test vector<bool>::iterator optimization
     assert(test_vector_bool(8));

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. test-suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants