Skip to content

Conversation

@mordante
Copy link
Member

@mordante mordante commented Jul 8, 2024

vector<bool>'s shrink_to_fit implementation is using the "swap-to-free-container-resources-trick" which only shrinks when the input vector is empty. Since the request to shrink_to_fit is non-binding, this is a valid implementation. It is not a high-quality implementation. Since vector<bool> is not a very popular container the implementation has not been changed and only a test to validate the non-growing property has been added.

This was discovered while investigating #95161

@mordante mordante requested a review from a team as a code owner July 8, 2024 10:52
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Jul 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 8, 2024

@llvm/pr-subscribers-libcxx

Author: Mark de Wever (mordante)

Changes

vector<bool>'s shrink_to_fit implementation is using the "swap-to-free-container-resources-trick" which only shrinks when the input vector is empty. Since the request to shrink_to_fit is non-binding, this is a valid implementation. It is not a high-quality implementation. Since vector<bool> is not a very popular container the implementation has not been changed and only a test to validate the non-growing property has been added.

This was discovered while investigating #95161


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

1 Files Affected:

  • (modified) libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp (+37)
diff --git a/libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp b/libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
index b39245cab7bf4..4c1c1c68ed1aa 100644
--- a/libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector.bool/shrink_to_fit.pass.cpp
@@ -39,9 +39,46 @@ TEST_CONSTEXPR_CXX20 bool tests()
     return true;
 }
 
+#if TEST_STD_VER >= 23
+std::size_t min_bytes = 1000;
+
+template <typename T>
+struct increasing_allocator {
+  using value_type       = T;
+  increasing_allocator() = default;
+  template <typename U>
+  increasing_allocator(const increasing_allocator<U>&) noexcept {}
+  std::allocation_result<T*> allocate_at_least(std::size_t n) {
+    std::size_t allocation_amount = n * sizeof(T);
+    if (allocation_amount < min_bytes)
+      allocation_amount = min_bytes;
+    min_bytes += 1000;
+    return {static_cast<T*>(::operator new(allocation_amount)), allocation_amount / sizeof(T)};
+  }
+  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==(increasing_allocator<T>, increasing_allocator<U>) {
+  return true;
+}
+
+// https://github.com/llvm/llvm-project/issues/95161
+void test_increasing_allocator() {
+  std::vector<bool, increasing_allocator<bool>> v;
+  v.push_back(1);
+  std::size_t capacity = v.capacity();
+  v.shrink_to_fit();
+  assert(v.capacity() <= capacity);
+  assert(v.size() == 1);
+}
+#endif // TEST_STD_VER >= 23
+
 int main(int, char**)
 {
     tests();
+    test_increasing_allocator();
 #if TEST_STD_VER > 17
     static_assert(tests());
 #endif

@mordante mordante force-pushed the review/vector_bool_shrink_to_fit branch from 566bc52 to facbd58 Compare July 8, 2024 15:31
mordante added 2 commits July 17, 2024 07:43
vector<bool>'s shrink_to_fit implementation is using the
"swap-to-free-container-resources-trick" which only shrinks when the input
vector is empty. Since the request to shrink_to_fit is non-binding, this
is a valid implementation. It is not a high-quality implementation. Since
vector<bool> is not a very popular container the implementation has not
been changed and only a test to validate the non-growing property has been
added.

This was discovered while investigating llvm#95161
@mordante mordante force-pushed the review/vector_bool_shrink_to_fit branch from facbd58 to f698e05 Compare July 17, 2024 06:02
@mordante mordante added this to the LLVM 19.X Release milestone Jul 20, 2024
@ldionne ldionne merged commit c2e4386 into llvm:main Jul 23, 2024
@ldionne
Copy link
Member

ldionne commented Jul 23, 2024

/cherry-pick c2e4386

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 23, 2024
`vector<bool>`'s shrink_to_fit implementation is using the
"swap-to-free-container-resources-trick" which only shrinks when the
input vector is empty. Since the request to shrink_to_fit is
non-binding, this is a valid implementation. It is not a high-quality
implementation. Since `vector<bool>` is not a very popular container the
implementation has not been changed and only a test to validate the
non-growing property has been added.

This was discovered while investigating llvm#95161.

(cherry picked from commit c2e4386)
@llvmbot
Copy link
Member

llvmbot commented Jul 23, 2024

/pull-request #100145

@mordante mordante deleted the review/vector_bool_shrink_to_fit branch July 23, 2024 17:01
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 24, 2024
`vector<bool>`'s shrink_to_fit implementation is using the
"swap-to-free-container-resources-trick" which only shrinks when the
input vector is empty. Since the request to shrink_to_fit is
non-binding, this is a valid implementation. It is not a high-quality
implementation. Since `vector<bool>` is not a very popular container the
implementation has not been changed and only a test to validate the
non-growing property has been added.

This was discovered while investigating llvm#95161.

(cherry picked from commit c2e4386)
yuxuanchen1997 pushed a commit that referenced this pull request Jul 25, 2024
Summary:
`vector<bool>`'s shrink_to_fit implementation is using the
"swap-to-free-container-resources-trick" which only shrinks when the
input vector is empty. Since the request to shrink_to_fit is
non-binding, this is a valid implementation. It is not a high-quality
implementation. Since `vector<bool>` is not a very popular container the
implementation has not been changed and only a test to validate the
non-growing property has been added.

This was discovered while investigating #95161.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251031
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants