[libc++] Rename __construct_one_at_end to __emplace_back_assume_capacity

[ldionne]

This makes it clear that the end of the vector is updated when calling the function.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

Also, simplify its implementation to avoid using _ConstructTransaction, which has a confusing interface and doesn't provide much value in this case. Indeed, if an exception is thrown when we construct the single element at the end, we simply don't need to call __annotate_increase.

---

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

1 Files Affected:

• (modified) libcxx/include/__vector/vector.h (+16-12)

diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 9155fb52a69b1..e8b9eded2c79e 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -461,6 +461,17 @@ class _LIBCPP_TEMPLATE_VIS vector {
   emplace_back(_Args&&... __args);
 #endif
 
+  template <class... _Args>
+  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __emplace_back_assume_capacity(_Args&&... __args) {
+    _LIBCPP_ASSERT_INTERNAL(
+        size() < capacity(), "We assume that we have enough space to insert an element at the end of the vector");
+    __alloc_traits::construct(this->__alloc_, std::__to_address(this->__end_), std::forward<_Args>(__args)...);
+    ++this->__end_;
+#if _LIBCPP_HAS_ASAN
+    __annotate_increase(1);
+#endif
+  }
+
 #if _LIBCPP_STD_VER >= 23
   template <_ContainerCompatibleRange<_Tp> _Range>
   _LIBCPP_HIDE_FROM_ABI constexpr void append_range(_Range&& __range) {
@@ -758,13 +769,6 @@ class _LIBCPP_TEMPLATE_VIS vector {
     _ConstructTransaction& operator=(_ConstructTransaction const&) = delete;
   };
 
-  template <class... _Args>
-  _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __construct_one_at_end(_Args&&... __args) {
-    _ConstructTransaction __tx(*this, 1);
-    __alloc_traits::construct(this->__alloc_, std::__to_address(__tx.__pos_), std::forward<_Args>(__args)...);
-    ++__tx.__pos_;
-  }
-
   _LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_HIDE_FROM_ABI void __base_destruct_at_end(pointer __new_last) _NOEXCEPT {
     pointer __soon_to_be_end = this->__end_;
     while (__new_last != __soon_to_be_end)
@@ -1152,7 +1156,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
     vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
   pointer __end = this->__end_;
   if (__end < this->__cap_) {
-    __construct_one_at_end(std::forward<_Args>(__args)...);
+    __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
     ++__end;
   } else {
     __end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
@@ -1206,7 +1210,7 @@ vector<_Tp, _Allocator>::insert(const_iterator __position, const_reference __x)
   pointer __p = this->__begin_ + (__position - begin());
   if (this->__end_ < this->__cap_) {
     if (__p == this->__end_) {
-      __construct_one_at_end(__x);
+      __emplace_back_assume_capacity(__x);
     } else {
       __move_range(__p, this->__end_, __p + 1);
       const_pointer __xr = pointer_traits<const_pointer>::pointer_to(__x);
@@ -1228,7 +1232,7 @@ vector<_Tp, _Allocator>::insert(const_iterator __position, value_type&& __x) {
   pointer __p = this->__begin_ + (__position - begin());
   if (this->__end_ < this->__cap_) {
     if (__p == this->__end_) {
-      __construct_one_at_end(std::move(__x));
+      __emplace_back_assume_capacity(std::move(__x));
     } else {
       __move_range(__p, this->__end_, __p + 1);
       *__p = std::move(__x);
@@ -1248,7 +1252,7 @@ vector<_Tp, _Allocator>::emplace(const_iterator __position, _Args&&... __args) {
   pointer __p = this->__begin_ + (__position - begin());
   if (this->__end_ < this->__cap_) {
     if (__p == this->__end_) {
-      __construct_one_at_end(std::forward<_Args>(__args)...);
+      __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
     } else {
       __temp_value<value_type, _Allocator> __tmp(this->__alloc_, std::forward<_Args>(__args)...);
       __move_range(__p, this->__end_, __p + 1);
@@ -1300,7 +1304,7 @@ vector<_Tp, _Allocator>::__insert_with_sentinel(const_iterator __position, _Inpu
   pointer __p           = this->__begin_ + __off;
   pointer __old_last    = this->__end_;
   for (; this->__end_ != this->__cap_ && __first != __last; ++__first)
-    __construct_one_at_end(*__first);
+    __emplace_back_assume_capacity(*__first);
 
   if (__first == __last)
     (void)std::rotate(__p, __old_last, this->__end_);

[winner245]

Thanks, LGTM. Previously, I found the name __construct_one_at_end somewhat confusing since we already have a similar member named __construct_at_end. Moreover, it wasn't immediately clear whether __construct_one_at_end updated this->__end_ without referring to its definition. With the new name __emplace_back_assume_capacity, it is now evident that the function is a special case of emplace_back, which would update this->__end_ upon success.

[winner245]

I analyzed the ASAN failure. It seems that the failure is caused by omitting an __annotate_shrink call when the element construction fails. Previously, ~_ConstructTransaction() ensured that when construction exception is thrown, this destructor would call __annotate_shrink(__new_end_ - __v_.__begin_);. This omission might be the cause of the ASAN failure.

[ldionne]

Thanks for taking a look! What seems to be actually happening is that if we fail to __annotate_increase(1) before we use construct_at, Asan thinks that we're writing outside of the container and complains. So we need to use __annotate_increase upon entry and then to use __anotate_shrink if the operation failed.

I'll return to the original implementation for now, but I still think the name change is an improvement.

[winner245]

The new name __emplace_back_assume_capacity is much clearer and more descriptive. While __construct_at_end is widely used in <vector>, __construct_one_at_end appears in 5 places. So I think we can replace the confusing name __construct_one_at_end by __emplace_back_assume_capacity to improve clarity.