[libcxx-commits] [libcxx] [libc++] Fix shrink_to_fit to swap buffer only when capacity is strictly smaller (PR #127321)
Peng Liu via libcxx-commits
libcxx-commits at lists.llvm.org
Sun Feb 16 13:12:07 PST 2025
https://github.com/winner245 updated https://github.com/llvm/llvm-project/pull/127321
>From 16ac54911fe1698a328bd3f15036ba2a34c83af4 Mon Sep 17 00:00:00 2001
From: Peng Liu <winner245 at hotmail.com>
Date: Sat, 15 Feb 2025 08:44:51 -0500
Subject: [PATCH 1/2] Fix shrink_to_fit to swap buffer only when capacity is
strictly smaller
---
libcxx/include/string | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libcxx/include/string b/libcxx/include/string
index 396e73522d3e7..eb5ffc65d03f8 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -3541,7 +3541,7 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
// 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 > capacity()) {
+ if (__allocation.count - 1 >= capacity()) {
__alloc_traits::deallocate(__alloc_, __allocation.ptr, __allocation.count);
return;
}
>From 370507290addba33db688761323585c8628af975 Mon Sep 17 00:00:00 2001
From: Peng Liu <winner245 at hotmail.com>
Date: Sun, 16 Feb 2025 16:10:15 -0500
Subject: [PATCH 2/2] Add tests to ensure no buffer swapping with equal alloc
size
---
.../string.capacity/shrink_to_fit.pass.cpp | 60 +++++++++----
libcxx/test/support/increasing_allocator.h | 86 +++++++++++++++++++
2 files changed, 129 insertions(+), 17 deletions(-)
diff --git a/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp b/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
index 2d901e7afe2b6..accf8f9e7db83 100644
--- a/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
+++ b/libcxx/test/std/strings/basic.string/string.capacity/shrink_to_fit.pass.cpp
@@ -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);
+ }
+ }
+
+ return true;
}
-#endif // TEST_STD_VER >= 23
int main(int, char**) {
test();
-#if TEST_STD_VER >= 23
- test_increasing_allocator();
-#endif
#if TEST_STD_VER > 17
static_assert(test());
#endif
diff --git a/libcxx/test/support/increasing_allocator.h b/libcxx/test/support/increasing_allocator.h
index 30bd6f40c8dad..9eaa3e1ec5bff 100644
--- a/libcxx/test/support/increasing_allocator.h
+++ b/libcxx/test/support/increasing_allocator.h
@@ -10,6 +10,7 @@
#define TEST_SUPPORT_INCREASING_ALLOCATOR_H
#include <cstddef>
+#include <limits>
#include <memory>
#include "test_macros.h"
@@ -49,4 +50,89 @@ TEST_CONSTEXPR_CXX20 bool operator==(increasing_allocator<T>, increasing_allocat
return true;
}
+template <std::size_t MinAllocSize, typename T>
+class min_size_allocator {
+public:
+ using value_type = T;
+ min_size_allocator() = default;
+
+ template <typename U>
+ TEST_CONSTEXPR_CXX20 min_size_allocator(const min_size_allocator<MinAllocSize, U>&) TEST_NOEXCEPT {}
+
+#if TEST_STD_VER >= 23
+ TEST_CONSTEXPR_CXX23 std::allocation_result<T*> allocate_at_least(std::size_t n) {
+ if (n < MinAllocSize)
+ n = MinAllocSize;
+ return std::allocator<T>{}.allocate_at_least(n);
+ }
+#endif // TEST_STD_VER >= 23
+
+ TEST_NODISCARD TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
+ if (n < MinAllocSize)
+ n = MinAllocSize;
+ return std::allocator<T>().allocate(n);
+ }
+
+ TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t n) TEST_NOEXCEPT { std::allocator<T>().deallocate(p, n); }
+
+ template <typename U>
+ struct rebind {
+ using other = min_size_allocator<MinAllocSize, U>;
+ };
+};
+
+template <std::size_t MinAllocSize, typename T, typename U>
+TEST_CONSTEXPR_CXX20 bool
+operator==(const min_size_allocator<MinAllocSize, T>&, const min_size_allocator<MinAllocSize, U>&) {
+ return true;
+}
+
+template <std::size_t MinAllocSize, typename T, typename U>
+TEST_CONSTEXPR_CXX20 bool
+operator!=(const min_size_allocator<MinAllocSize, T>&, const min_size_allocator<MinAllocSize, U>&) {
+ return false;
+}
+
+template <typename T>
+class pow2_allocator {
+public:
+ using value_type = T;
+ pow2_allocator() = default;
+
+ template <typename U>
+ TEST_CONSTEXPR_CXX20 pow2_allocator(const pow2_allocator<U>&) TEST_NOEXCEPT {}
+
+#if TEST_STD_VER >= 23
+ TEST_CONSTEXPR_CXX23 std::allocation_result<T*> allocate_at_least(std::size_t n) {
+ return std::allocator<T>{}.allocate_at_least(next_power_of_two(n));
+ }
+#endif // TEST_STD_VER >= 23
+
+ TEST_NODISCARD TEST_CONSTEXPR_CXX20 T* allocate(std::size_t n) {
+ return std::allocator<T>().allocate(next_power_of_two(n));
+ }
+
+ TEST_CONSTEXPR_CXX20 void deallocate(T* p, std::size_t n) TEST_NOEXCEPT { std::allocator<T>().deallocate(p, n); }
+
+private:
+ TEST_CONSTEXPR_CXX20 std::size_t next_power_of_two(std::size_t n) const {
+ if ((n & (n - 1)) == 0)
+ return n;
+ for (std::size_t shift = 1; shift < std::numeric_limits<std::size_t>::digits; shift <<= 1) {
+ n |= n >> shift;
+ }
+ return n + 1;
+ }
+};
+
+template <typename T, typename U>
+TEST_CONSTEXPR_CXX20 bool operator==(const pow2_allocator<T>&, const pow2_allocator<U>&) {
+ return true;
+}
+
+template <typename T, typename U>
+TEST_CONSTEXPR_CXX20 bool operator!=(const pow2_allocator<T>&, const pow2_allocator<U>&) {
+ return false;
+}
+
#endif // TEST_SUPPORT_INCREASING_ALLOCATOR_H
More information about the libcxx-commits
mailing list