[libcxx-commits] [libcxx] [libc++] Simplify vector<bool>::__construct_at_end (Reopend) (PR #119632)

[Peng Liu via libcxx-commits]

================
@@ -536,30 +536,20 @@ vector<bool, _Allocator>::__recommend(size_type __new_size) const {
 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_;
+  iterator __old_end = end();
   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);
+  this->__begin_[(this->__size_ - 1) / __bits_per_word] = __storage_type(0);
----------------
winner245 wrote:

Great catch! Now I fully understand you from the code you provided. Your concern is indeed valid if user code is allowed to directly call `__construct_at_end`. In that case, overwriting the first few bits currently used in the last word would be problematic. This was overlooked in my refactor, as well as in the previous implementation, because `__construct_at_end` is only used by constructors, `__assign_with_size`, and `reserve`. When used inside constructors, we have an empty vector, so nothing got overwritten. For the usage in `__assign_with_size`, `__construct_at_end` is called after a `clear()` call, meaning we have an empty vector before the call. Moreover, since it's used for assignments, overwriting some cleared bits is not an issue. In `reserve`, `__v.__construct_at_end(this->begin(), this->end(), this->size())` is called on a newly created empty vector `__v`, so it's fine. 

So basically, all the existing usages of `__construct_at_end` implicitly requires that the vector is empty. After realizing this, I realized that the additional precondition you mentioned, that the vector must be empty before calling `__construct_at_end`, is valid. This precondition was previously ignored but hasn't led to issues because all existing calls satisfy it.

With this additional precondition you pointed out, we actually have two preconditions for this function `__construct_at_end(, , __n)`: 
- `_LIBCPP_ASSERT_INTERNAL(empty(), "this function requires an empty vector");`
- `_LIBCPP_ASSERT_VALID_INPUT_RANGE(__n > 0, "invalid range specified");`

Adding the above two checks in `__construct_at_end` is a great fix. 

I also have another implementation for `__construct_at_end` which seems to remove both preconditions: 

```cpp
void __construct_at_end(_InputIterator __first, _Sentinel __last, size_type __n) {
  std::__copy(__first, __last, end());
  this->__size_ += __n;
  if (end().__ctz_ != 0) // has uninitialized trailing bits in the last word
    std::fill_n(end(), __bits_per_word - end().__ctz_, 0);
}
```

This implementation only writes to the trailing bits in the last word, not overwriting the existing leading bits. Also, if `__n == 0` (i.e., `__first == __last`), this function does not do anything. 

What do you think about this approach?


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