[libc++] Fix throwing away smaller allocations in string::shrink_to_fit

[philnik777]

Currently string::shrink_to_fit() throws away any allocations which return more capacity than we requested, even if that allocation is still smaller than the current capacity. This patch fixes this to compare the returned allocation against the current capacity of the string instead of against the requested capacity.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

Currently string::shrink_to_fit() throws away any allocations which return more capacity than we requested, even if that allocation is still smaller than the current capacity. This patch fixes this to compare the retuend allocation against the current capacity of the string instead of against the requested capacity.

---

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

2 Files Affected:

• (modified) libcxx/include/string (+1-1)
• (added) libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp (+55)

diff --git a/libcxx/include/string b/libcxx/include/string
index b1cedbe68f7956..1034be8fdad8b3 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -3393,7 +3393,7 @@ basic_string<_CharT, _Traits, _Allocator>::__shrink_or_extend(size_type __target
         // The Standard mandates shrink_to_fit() does not increase the capacity.
         // With equal capacity keep the existing buffer. This avoids extra work
         // due to swapping the elements.
-        if (__allocation.count - 1 > __target_capacity) {
+        if (__allocation.count - 1 > capacity()) {
           __alloc_traits::deallocate(__alloc_, __allocation.ptr, __allocation.count);
           __annotate_new(__sz); // Undoes the __annotate_delete()
           return;
diff --git a/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp b/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
new file mode 100644
index 00000000000000..04a3f4cb2f06f4
--- /dev/null
+++ b/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
@@ -0,0 +1,55 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// <string>
+
+// void shrink_to_fit(); // constexpr since C++20
+
+// Make sure we use an allocation returned by allocate_at_least if it is smaller than the current allocation
+// even if it contains more bytes than we requested
+
+#include <cassert>
+#include <string>
+
+template <typename T>
+struct oversizing_allocator {
+  using value_type       = T;
+  oversizing_allocator() = default;
+  template <typename U>
+  oversizing_allocator(const oversizing_allocator<U>&) noexcept {}
+  std::allocation_result<T*> allocate_at_least(std::size_t n) {
+    ++n;
+    return {static_cast<T*>(::operator new(n * sizeof(T))), n};
+  }
+  T* allocate(std::size_t n) { return allocate_at_least(n).ptr; }
+  void deallocate(T* p, std::size_t) noexcept { ::operator delete(static_cast<void*>(p)); }
+};
+
+template <typename T, typename U>
+bool operator==(oversizing_allocator<T>, oversizing_allocator<U>) {
+  return true;
+}
+
+void test_oversizing_allocator() {
+  std::basic_string<char, std::char_traits<char>, oversizing_allocator<char>> s{
+      "String does not fit in the internal buffer and is a bit longer"};
+  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);
+}
+
+int main(int, char**) {
+  test_oversizing_allocator();
+
+  return 0;
+}

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 6b21cf8ccad84e2670e458d8bdaccbd0ae37b46b 0de7f619eddd25eb38c6c06d4ffd1eb851eb584b --extensions ,cpp -- libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp libcxx/include/string

View the diff from clang-format here.

diff --git a/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp b/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
index 04a3f4cb2f..73b70d6f10 100644
--- a/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
+++ b/libcxx/test/libcxx/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
@@ -40,7 +40,7 @@ bool operator==(oversizing_allocator<T>, oversizing_allocator<U>) {
 void test_oversizing_allocator() {
   std::basic_string<char, std::char_traits<char>, oversizing_allocator<char>> s{
       "String does not fit in the internal buffer and is a bit longer"};
-  s = "String does not fit in the internal buffer";
+  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();

[ldionne]

Isn't this something we can check for all implementations? I don't think this test is libc++ specific?

[philnik777]

Implementations aren't required to do anything when shrink_to_fit() is called. void shrink_to_fit() {} is a valid implementation: http://eel.is/c++draft/basic.string#string.capacity-16