[libc++] Simplify vector<bool>::__construct_at_end implementation

[winner245]

This PR proposes removing the redundant initialization of storage bits in both overloads of __construct_at_end. This function serves as a useful helper for various constructors, assign, and reserve methods of the class. The specific code segment in question is:

if (__old_size == 0 || ((__old_size - 1) / __bits_per_word) != ((this->__size_ - 1) / __bits_per_word)) {
  if (this->__size_ <= __bits_per_word)
    this->__begin_[0] = __storage_type(0);
  else
    this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
}

Rationale for Removal:
The purpose of the above code is to set every bit in the last storage word of __storage_type to 0 when the new size crosses the boundary of the current last storage word. However, setting the raw bits to 0 before an immediate assignment is completely unnecessary, as explained below:

• Redundancy: The initialization of the last storage word to zero is immediately overwritten by the subsequent call to std::fill_n. Therefore, even without this step, the uninitialized bits in the range [0, __size_) will be correctly set by the std::fill_n call, ensuring that all bits of interest are properly set.

• Safety against out-of-bounds access: The std::fill_n function itself operates within the valid range of the vector. Leaving the bits in the last storage word unset does not introduce any risk, as they remain out of bounds for vector<bool>.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Peng Liu (winner245)

Changes

Following #119502, I realized that vector<bool>::__construct_at_end performs unnecessary work. This PR proposes removing the redundant initialization of storage bits in both overloads of __construct_at_end. This function serves as a useful helper for various constructors, assign, and reserve methods of the class. The specific code segment in question is:

if (__old_size == 0 || ((__old_size - 1) / __bits_per_word) != ((this->__size_ - 1) / __bits_per_word)) {
  if (this->__size_ <= __bits_per_word)
    this->__begin_[0] = __storage_type(0);
  else
    this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
}


```cpp
if (__old_size == 0 || ((__old_size - 1) / __bits_per_word) != ((this->__size_ - 1) / __bits_per_word)) {
  if (this->__size_ <= __bits_per_word)
    this->__begin_[0] = __storage_type(0);
  else
    this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
}

Rationale for Removal:
The purpose of the above code is to set every bit in the last storage word of __storage_type to 0 when the new size crosses the boundary of the current last storage word. However, setting the raw bits to 0 before an immediate assignment is completely unnecessary, as explained below:

• Redundancy: The initialization of the last storage word to zero is immediately overwritten by the subsequent call to std::fill_n. Therefore, even without this step, the uninitialized bits in the range [0, __size_) will be correctly set by the std::fill_n call, ensuring that all bits of interest are properly set.

• Safety against out-of-bounds access: The std::fill_n function itself operates within the valid range of the vector. Leaving the bits in the last storage word unset does not introduce any risk, as they remain out of bounds for vector<bool>.

The existing comprehensive tests for the constructors, assign, and reserve functions continue to pass successfully after the removal of the above code, thereby validating the changes in this PR.

---

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

1 Files Affected:

• (modified) libcxx/include/__vector/vector_bool.h (+14-35)

diff --git a/libcxx/include/__vector/vector_bool.h b/libcxx/include/__vector/vector_bool.h
index 36eb7f350ac406..bc3411c4d97791 100644
--- a/libcxx/include/__vector/vector_bool.h
+++ b/libcxx/include/__vector/vector_bool.h
@@ -438,10 +438,22 @@ class _LIBCPP_TEMPLATE_VIS vector<bool, _Allocator> {
     return (__new_size + (__bits_per_word - 1)) & ~((size_type)__bits_per_word - 1);
   }
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type __recommend(size_type __new_size) const;
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __construct_at_end(size_type __n, bool __x);
+
+  //  Default constructs __n objects starting at __end_
+  //  Precondition:  __n > 0
+  //  Precondition:  size() + __n <= capacity()
+  //  Postcondition:  size() == size() + __n
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __construct_at_end(size_type __n, bool __x) {
+    std::fill_n(end(), __n, __x);
+    this->__size_ += __n;
+  }
   template <class _InputIterator, class _Sentinel>
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void
-  __construct_at_end(_InputIterator __first, _Sentinel __last, size_type __n);
+  __construct_at_end(_InputIterator __first, _Sentinel __last, size_type __n) {
+    std::__copy(__first, __last, end());
+    this->__size_ += __n;
+  }
+
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void __append(size_type __n, const_reference __x);
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 reference __make_ref(size_type __pos) _NOEXCEPT {
     return reference(__begin_ + __pos / __bits_per_word, __storage_type(1) << __pos % __bits_per_word);
@@ -530,39 +542,6 @@ vector<bool, _Allocator>::__recommend(size_type __new_size) const {
   return std::max(2 * __cap, __align_it(__new_size));
 }
 
-//  Default constructs __n objects starting at __end_
-//  Precondition:  __n > 0
-//  Precondition:  size() + __n <= capacity()
-//  Postcondition:  size() == size() + __n
-template <class _Allocator>
-inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 void
-vector<bool, _Allocator>::__construct_at_end(size_type __n, bool __x) {
-  size_type __old_size = this->__size_;
-  this->__size_ += __n;
-  if (__old_size == 0 || ((__old_size - 1) / __bits_per_word) != ((this->__size_ - 1) / __bits_per_word)) {
-    if (this->__size_ <= __bits_per_word)
-      this->__begin_[0] = __storage_type(0);
-    else
-      this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
-  }
-  std::fill_n(__make_iter(__old_size), __n, __x);
-}
-
-template <class _Allocator>
-template <class _InputIterator, class _Sentinel>
-_LIBCPP_CONSTEXPR_SINCE_CXX20 void
-vector<bool, _Allocator>::__construct_at_end(_InputIterator __first, _Sentinel __last, size_type __n) {
-  size_type __old_size = this->__size_;
-  this->__size_ += __n;
-  if (__old_size == 0 || ((__old_size - 1) / __bits_per_word) != ((this->__size_ - 1) / __bits_per_word)) {
-    if (this->__size_ <= __bits_per_word)
-      this->__begin_[0] = __storage_type(0);
-    else
-      this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
-  }
-  std::__copy(__first, __last, __make_iter(__old_size));
-}
-
 template <class _Allocator>
 inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 vector<bool, _Allocator>::vector()
     _NOEXCEPT_(is_nothrow_default_constructible<allocator_type>::value)

[philnik777]

This looks to me like you're introducing an uninitialized load, and the MSan CI seems to agree with me.

[winner245]

You are right. I am just about to close.