Skip to content

[libc++] Refactor basic_string::__recommend #162631

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[libc++] Refactor basic_string::__recommend #162631

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 19 additions & 18 deletions libcxx/include/string
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2249,7 +2249,9 @@ private:
// Allocate a buffer of __capacity size with __alloc and return it
_LIBCPP_HIDE_FROM_ABI static _LIBCPP_CONSTEXPR_SINCE_CXX20 __long
__allocate_long_buffer(_Allocator& __alloc, size_type __capacity) {
auto __buffer = std::__allocate_at_least(__alloc, __recommend(__capacity) + 1);
_LIBCPP_ASSERT_INTERNAL(!__fits_in_sso(__capacity),
"Trying to allocate long buffer for a capacity what would fit into the small buffer");
auto __buffer = std::__allocate_at_least(__alloc, __align_allocation_size(__capacity));

if (__libcpp_is_constant_evaluated()) {
for (size_type __i = 0; __i != __buffer.count; ++__i)
Expand Down Expand Up @@ -2347,16 +2349,17 @@ private:
return (__s + (__a - 1)) & ~(__a - 1);
}
enum { __alignment = 8 };
static _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type __recommend(size_type __s) _NOEXCEPT {
if (__s < __min_cap) {
return static_cast<size_type>(__min_cap) - 1;
}

_LIBCPP_HIDE_FROM_ABI static _LIBCPP_CONSTEXPR_SINCE_CXX20 size_type
__align_allocation_size(size_type __size) _NOEXCEPT {
Copy link
Member

Choose a reason for hiding this comment

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

This is a great opportunity to add a comment explaining what this function does, and most interestingly, why it is needed. Perhaps some of that can be offloaded to the explanation of __endian_factor at the top of the class.

_LIBCPP_ASSERT_INTERNAL(
!__fits_in_sso(__size), "Trying to align allocation of a size which would fit into the SSO");
const size_type __boundary = sizeof(value_type) < __alignment ? __alignment / sizeof(value_type) : __endian_factor;
size_type __guess = __align_it<__boundary>(__s + 1) - 1;
if (__guess == __min_cap)
size_type __guess = __align_it<__boundary>(__size + 1);
Copy link
Member

Choose a reason for hiding this comment

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

Why is __guess one unit larger than it was before?

if (__guess == __min_cap + 1)
__guess += __endian_factor;

_LIBCPP_ASSERT_INTERNAL(__guess >= __s, "recommendation is below the requested size");
_LIBCPP_ASSERT_INTERNAL(__guess >= __size, "aligned allocation size is below the requested size");
return __guess;
}

Expand Down Expand Up @@ -2694,8 +2697,7 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::__
if (__delta_cap > __ms - __old_cap)
__throw_length_error();
pointer __old_p = __get_pointer();
size_type __cap =
__old_cap < __ms / 2 - __alignment ? __recommend(std::max(__old_cap + __delta_cap, 2 * __old_cap)) : __ms;
size_type __cap = __old_cap < __ms / 2 - __alignment ? std::max(__old_cap + __delta_cap, 2 * __old_cap) : __ms;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you not call __recommend (__align_allocation_size) anymore?

__annotate_delete();
auto __guard = std::__make_scope_guard(__annotate_new_size(*this));
__long __buffer = __allocate_long_buffer(__alloc_, __cap);
Expand Down Expand Up @@ -2732,8 +2734,7 @@ _LIBCPP_DEPRECATED_("use __grow_by_without_replace") basic_string<_CharT, _Trait
if (__delta_cap > __ms - __old_cap)
this->__throw_length_error();
pointer __old_p = __get_pointer();
size_type __cap =
__old_cap < __ms / 2 - __alignment ? __recommend(std::max(__old_cap + __delta_cap, 2 * __old_cap)) : __ms;
size_type __cap = __old_cap < __ms / 2 - __alignment ? std::max(__old_cap + __delta_cap, 2 * __old_cap) : __ms;
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

__long __buffer = __allocate_long_buffer(__alloc_, __cap);
if (__n_copy != 0)
traits_type::copy(std::__to_address(__buffer.__data_), std::__to_address(__old_p), __n_copy);
Expand Down Expand Up @@ -3399,25 +3400,25 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::re

template <class _CharT, class _Traits, class _Allocator>
inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::shrink_to_fit() _NOEXCEPT {
size_type __target_capacity = __recommend(size());
if (__target_capacity == capacity())
if (!__is_long())
return;

_LIBCPP_ASSERT_INTERNAL(__is_long(), "Trying to shrink small string");

// We're a long string and we're shrinking into the small buffer.
const auto __ptr = __get_long_pointer();
const auto __size = __get_long_size();
const auto __cap = __get_long_cap();

if (__fits_in_sso(__target_capacity)) {
// We're a long string and we're shrinking into the small buffer.
if (__fits_in_sso(__size)) {
__annotation_guard __g(*this);
__set_short_size(__size);
traits_type::copy(std::__to_address(__get_short_pointer()), std::__to_address(__ptr), __size + 1);
__alloc_traits::deallocate(__alloc_, __ptr, __cap);
return;
}

if (__align_allocation_size(__size) == __cap)
Copy link
Member

Choose a reason for hiding this comment

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

IIUC we'll end up computing this again inside __allocate_long_buffer, right? That's a bit annoying, but perhaps the compiler's able to fold everything if it inlines __allocate_long_buffer. Did you check whether it does?

return;

# if _LIBCPP_HAS_EXCEPTIONS
try {
# endif // _LIBCPP_HAS_EXCEPTIONS
Expand Down
Loading