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

[Peng Liu 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);
+    }
+  }
----------------
winner245 wrote:

> 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.

I agree that they serve different purposes, but the way the tests are written and the fact that they use similar special allocators are the same. Therefore, grouping these tests together seems natural to me.

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

Because these tests also include assertions for standard requirements, such as `assert(s.capacity() <= capacity)`. It's crucial to ensure this requirement is met for `shrink_to_fit`, especially for the special allocators considered here. Once the standard requirement is met, the libc++ specific tests become meaningful. Also, I've used the `LIBCPP_ASSERT` macro, which ensures no impact on other non-libc++ implementations. So placing the tests here seems a reasonable choice.  

> What I don't understand is why you're not assuming that the case you're testing is actually happening.

I placed the `LIBCPP_ASSERT(s.data() == data)` assertion within the `if (s.capacity() == capacity)` branch minimizes impact on non-libc++ implementations. However, I do see your concern here.  So we can make the tests explicit for libc++ implementation as follows: 

```cpp
LIBCPP_ASSERT(s.capacity() == capacity && s.data() == data);
```

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