[libc++] Fix no-op shrink_to_fit for vector<bool>

[winner245]

This PR addresses an issue where the shrink_to_fit function in vector<bool> is effectively a no-op, meaning it will never shrink the capacity.

Fixes #122502.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

The current implementation of vector<bool>::shrink_to_fit contains a logical error that prevents the function from doing anything practical, i.e., it will never shrink the capacity unless an arithmetic underflow happens. This is because the implementation checks the following condition

if (__external_cap_to_internal(size()) > __cap_) {
  try {
    do_shrink();
  } catch (...) {
  }  
}

However, this condition is always false (with one exception), as the number of used internal words __external_cap_to_internal(size()) will never exceed the internal storage capacity (__cap_). The only exception for this condition to be true is when size() == 0, where the evaluation of __external_cap_to_internal(0) underflows and wraps around to become size_type(-1). But it is highly unlikely that this behavior was intended, because it relies on an arithmetic underflow. Even if it were intentional, a more straightforward approach would have been:

if (empty())
  deallocate();

which avoids the try/catch clause and becomes much cleaner and simpler.

While the current implementation technically conforms to the standard—since the shrink-to-fit request is non-binding—it appears to be a logical error to me, particularly since a similar logical error was found in the reserve() function of __split_buffer in #105681, which remained uncorrected until last month (#115735).

This PR addresses the issue by modifying shrink_to_fit to actually perform the necessary capacity reduction, ensuring it behaves as expected.

---

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

1 Files Affected:

• (modified) libcxx/include/__vector/vector_bool.h (+4-2)

diff --git a/libcxx/include/__vector/vector_bool.h b/libcxx/include/__vector/vector_bool.h
index 36eb7f350ac406..190ea2585b7821 100644
--- a/libcxx/include/__vector/vector_bool.h
+++ b/libcxx/include/__vector/vector_bool.h
@@ -841,11 +841,13 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::reserve(size_type _
 
 template <class _Allocator>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 void vector<bool, _Allocator>::shrink_to_fit() _NOEXCEPT {
-  if (__external_cap_to_internal(size()) > __cap_) {
+  if (__external_cap_to_internal(size()) < __cap_) {
 #if _LIBCPP_HAS_EXCEPTIONS
     try {
 #endif // _LIBCPP_HAS_EXCEPTIONS
-      vector(*this, allocator_type(__alloc_)).swap(*this);
+      vector __v(*this, allocator_type(__alloc_));
+      if (__v.__cap_ < __cap_)
+        __v.swap(*this);
 #if _LIBCPP_HAS_EXCEPTIONS
     } catch (...) {
     }

[ldionne]

The part of the test suite located under libcxx/test/std is intended to be portable and usable by other implementations (like MSVC STL or libstdc++). As such, we don't use things specific to libc++ like C::__storage_type. Or if we absolutely must, we either:

1. Put that part of the test under libcxx/test/libcxx, which contains tests that are not entirely portable, or
2. Guard the check with #if defined(_LIBCPP_VERSION), or
3. Use something like LIBCPP_ASSERT from test_macros.h.

Which of those three we do depends on the amount of libc++ specific test code we're trying to add, maintenance burden, etc.

In this case, the very best would be to figure out the number of bits per word via some public-facing API if that's doable, but I'm not certain it is. Otherwise, I would probably go for #if defined(_LIBCPP_VERSION) around those two tests.

[winner245]

Thank you for your insightful feedback. The three options you mentioned have provided me with some new and valuable information that I wasn't previously aware of.

Among the options, I believe using #if defined(_LIBCPP_VERSION) around the two tests that use C::__storage_type would be the most appropriate for my purpose. This is because libc++ and libstdc++ have different word sizes (libc++ uses Alloc::size_type, which is typically 64 bits, while libstdc++ uses unsigned long). Moreover, I don't find a way to extract this information from a public API. Even if I could, the decision on whether to shrink would be implementation-dependent as the request to shrink is non-binding. Therefore, I choose to use the #if defined(_LIBCPP_VERSION) option you suggested.