[libcxx-commits] [libcxx] [libc++] Fix shrink_to_fit to swap buffer only when capacity is strictly smaller (PR #127321)

[Nikolas Klauser via libcxx-commits]

================
@@ -61,28 +61,54 @@ TEST_CONSTEXPR_CXX20 bool test() {
   test_string<std::basic_string<char, std::char_traits<char>, safe_allocator<char>>>();
 #endif
 
-  return true;
-}
-
 #if TEST_STD_VER >= 23
-// https://github.com/llvm/llvm-project/issues/95161
-void test_increasing_allocator() {
-  std::basic_string<char, std::char_traits<char>, increasing_allocator<char>> s{
-      "String does not fit in the internal buffer"};
-  std::size_t capacity = s.capacity();
-  std::size_t size     = s.size();
-  s.shrink_to_fit();
-  assert(s.capacity() <= capacity);
-  assert(s.size() == size);
-  LIBCPP_ASSERT(is_string_asan_correct(s));
+  { // Make sure shrink_to_fit never increases capacity
+    // See: https://github.com/llvm/llvm-project/issues/95161
+    std::basic_string<char, std::char_traits<char>, increasing_allocator<char>> s{
+        "String does not fit in the internal buffer"};
+    std::size_t capacity = s.capacity();
+    std::size_t size     = s.size();
+    s.shrink_to_fit();
+    assert(s.capacity() <= capacity);
+    assert(s.size() == size);
+    LIBCPP_ASSERT(is_string_asan_correct(s));
+  }
+#endif
+
+  {   // Ensure that the libc++ implementation of shrink_to_fit does NOT swap buffer with equal allocation sizes
+    { // Test with custom allocator with a minimum allocation size
+      std::basic_string<char, std::char_traits<char>, min_size_allocator<128, char> > s(
+          "A long string exceeding SSO limit but within min alloc size");
+      std::size_t capacity = s.capacity();
+      std::size_t size     = s.size();
+      auto data            = s.data();
+      s.shrink_to_fit();
+      assert(s.capacity() <= capacity);
+      assert(s.size() == size);
+      LIBCPP_ASSERT(is_string_asan_correct(s));
+      if (s.capacity() == capacity)
+        LIBCPP_ASSERT(s.data() == data);
+    }
+    { // Test with custom allocator with a minimum power of two allocation size
+      std::basic_string<char, std::char_traits<char>, pow2_allocator<char> > s(
+          "This is a long string that exceeds the SSO limit");
+      std::size_t capacity = s.capacity();
+      std::size_t size     = s.size();
+      auto data            = s.data();
+      s.shrink_to_fit();
+      assert(s.capacity() <= capacity);
+      assert(s.size() == size);
+      LIBCPP_ASSERT(is_string_asan_correct(s));
+      if (s.capacity() == capacity)
+        LIBCPP_ASSERT(s.data() == data);
+    }
+  }
----------------
philnik777 wrote:

> The test with `increasing_alloctor` was already here, and my tests are the same type of tests.

I don't think that's correct. the `increasing_allocator` test is a requirement from the standard. The new tests only test implementation-specific behaviour AFAICT.

That's why I put them here. Could you please clarify why you think they should be moved to a different `shrink_to_fit.pass.cpp` file?

Because they are non-portable. Unless there is a reason not to, non-portable tests should live inside `test/libcxx`

> > I don't think we test anything useful that's not implementation-specific here. I also don't understand why you have the `if (s.capactiy() == capacity)`, sine that's the whole point of the test.
> 
> The two new test cases are designed to test a scenario where the new allocation size is equal to the existing buffer capacity, and `if (s.capacity() == capacity)` is detecting this scenario. The `LIBCPP_ASSERT(s.data() == data)` ensures that the libc++ implementation does not swap the buffer in this scenario, which is implementation-specific. I believe these tests are useful as they successfully detect this QoI matter.

The whole point of the tests is to check implementation-specific behaviour and the `if` makes it really unclear whether this test is actually testing what it's supposed to. What I don't understand is why you're not assuming that the case you're testing is actually happening.


https://github.com/llvm/llvm-project/pull/127321