[libcxx-commits] [libcxx] [libc++] Fix shrink_to_fit implementation for vector<bool> (PR #120495)

[via libcxx-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

<details>
<summary>Changes</summary>

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

```cpp
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:

```cpp
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
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 (...) {
     }

``````````

</details>


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