Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[libc++] Rename __construct_one_at_end to __emplace_back_assume_capacity #132276

Merged
merged 2 commits into from
Mar 25, 2025
Merged

[libc++] Rename __construct_one_at_end to __emplace_back_assume_capacity #132276

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
951ae13
[libc++] Rename __construct_one_at_end to __emplace_back_assume_capacity
ldionne Nov 1, 2024
d38b35d
Return to using _ConstructTransaction
ldionne Mar 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions libcxx/include/__vector/vector.h
View file
Original file line number Diff line number Diff line change
@@ -461,6 +461,15 @@ 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");
_ConstructTransaction __tx(*this, 1);
__alloc_traits::construct(this->__alloc_, std::__to_address(__tx.__pos_), std::forward<_Args>(__args)...);
++__tx.__pos_;
}

#if _LIBCPP_STD_VER >= 23
template <_ContainerCompatibleRange<_Tp> _Range>
_LIBCPP_HIDE_FROM_ABI constexpr void append_range(_Range&& __range) {
@@ -758,13 +767,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 +1154,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)...);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@winner245 winner245 Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

++__end;
} else {
__end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
@@ -1206,7 +1208,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 +1230,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 +1250,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 +1302,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_);