[libc++][NFC] Simplify string a bit #127135
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
winner245
force-pushed
the
simplify-string
branch
4 times, most recently
from
9e89ce6 to
919ec35
Compare
winner245
added
the
libc++
Member
|
@llvm/pr-subscribers-libcxx Author: Peng Liu (winner245) ChangesThis PR refactors
Full diff: https://github.com/llvm/llvm-project/pull/127135.diff 1 Files Affected:
diff --git a/libcxx/include/string b/libcxx/include/string
index b280f5f458459..41cc082d94815 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -2335,7 +2335,7 @@ private:
if (!__str.__is_long()) {
if (__is_long()) {
__annotate_delete();
- __alloc_traits::deallocate(__alloc_, __get_long_pointer(), capacity() + 1);
+ __alloc_traits::deallocate(__alloc_, __get_long_pointer(), __get_long_cap());
__rep_ = __rep();
}
__alloc_ = __str.__alloc_;
@@ -2350,7 +2350,7 @@ private:
__alloc_ = std::move(__a);
__set_long_pointer(__allocation.ptr);
__set_long_cap(__allocation.count);
- __set_long_size(__str.size());
+ __set_long_size(__str.__get_long_size());
}
}
}
@@ -2470,8 +2470,8 @@ template <class _CharT,
class _Traits,
class _Allocator = allocator<_CharT>,
class = enable_if_t<__is_allocator<_Allocator>::value> >
-explicit basic_string(basic_string_view<_CharT, _Traits>,
- const _Allocator& = _Allocator()) -> basic_string<_CharT, _Traits, _Allocator>;
+explicit basic_string(basic_string_view<_CharT, _Traits>, const _Allocator& = _Allocator())
+ -> basic_string<_CharT, _Traits, _Allocator>;
template <class _CharT,
class _Traits,
@@ -2758,20 +2758,19 @@ template <class _CharT, class _Traits, class _Allocator>
template <bool __is_short>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NOINLINE basic_string<_CharT, _Traits, _Allocator>&
basic_string<_CharT, _Traits, _Allocator>::__assign_no_alias(const value_type* __s, size_type __n) {
- size_type __cap = __is_short ? static_cast<size_type>(__min_cap) : __get_long_cap();
- if (__n < __cap) {
- size_type __old_size = __is_short ? __get_short_size() : __get_long_size();
+ size_type __cap = capacity();
+ size_type __old_size = size();
+ if (__n <= __cap) {
if (__n > __old_size)
__annotate_increase(__n - __old_size);
- pointer __p = __is_short ? __get_short_pointer() : __get_long_pointer();
- __is_short ? __set_short_size(__n) : __set_long_size(__n);
+ pointer __p =
+ __is_short ? (__set_short_size(__n), __get_short_pointer()) : (__set_long_size(__n), __get_long_pointer());
traits_type::copy(std::__to_address(__p), __s, __n);
traits_type::assign(__p[__n], value_type());
if (__old_size > __n)
__annotate_shrink(__old_size);
} else {
- size_type __sz = __is_short ? __get_short_size() : __get_long_size();
- __grow_by_and_replace(__cap - 1, __n - __cap + 1, __sz, 0, __sz, __n, __s);
+ __grow_by_and_replace(__cap, __n - __cap, __old_size, 0, __old_size, __n, __s);
}
return *this;
}
@@ -2779,17 +2778,16 @@ basic_string<_CharT, _Traits, _Allocator>::__assign_no_alias(const value_type* _
template <class _CharT, class _Traits, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 _LIBCPP_NOINLINE basic_string<_CharT, _Traits, _Allocator>&
basic_string<_CharT, _Traits, _Allocator>::__assign_external(const value_type* __s, size_type __n) {
- size_type __cap = capacity();
+ size_type __cap = capacity();
+ size_type __old_size = size();
if (__cap >= __n) {
- size_type __old_size = size();
if (__n > __old_size)
__annotate_increase(__n - __old_size);
value_type* __p = std::__to_address(__get_pointer());
traits_type::move(__p, __s, __n);
return __null_terminate_at(__p, __n);
} else {
- size_type __sz = size();
- __grow_by_and_replace(__cap, __n - __cap, __sz, 0, __sz, __n, __s);
+ __grow_by_and_replace(__cap, __n - __cap, __old_size, 0, __old_size, __n, __s);
return *this;
}
}
@@ -2807,8 +2805,7 @@ basic_string<_CharT, _Traits, _Allocator>::assign(size_type __n, value_type __c)
size_type __cap = capacity();
size_type __old_size = size();
if (__cap < __n) {
- size_type __sz = size();
- __grow_by_without_replace(__cap, __n - __cap, __sz, 0, __sz);
+ __grow_by_without_replace(__cap, __n - __cap, __old_size, 0, __old_size);
__annotate_increase(__n);
} else if (__n > __old_size)
__annotate_increase(__n - __old_size);
@@ -2820,17 +2817,10 @@ basic_string<_CharT, _Traits, _Allocator>::assign(size_type __n, value_type __c)
template <class _CharT, class _Traits, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string<_CharT, _Traits, _Allocator>&
basic_string<_CharT, _Traits, _Allocator>::operator=(value_type __c) {
- pointer __p;
size_type __old_size = size();
if (__old_size == 0)
__annotate_increase(1);
- if (__is_long()) {
- __p = __get_long_pointer();
- __set_long_size(1);
- } else {
- __p = __get_short_pointer();
- __set_short_size(1);
- }
+ pointer __p = __is_long() ? (__set_long_size(1), __get_long_pointer()) : (__set_short_size(1), __get_short_pointer());
traits_type::assign(*__p, __c);
traits_type::assign(*++__p, value_type());
if (__old_size > 1)
@@ -2846,8 +2836,8 @@ basic_string<_CharT, _Traits, _Allocator>::operator=(const basic_string& __str)
if (!__is_long()) {
if (!__str.__is_long()) {
size_type __old_size = __get_short_size();
- if (__get_short_size() < __str.__get_short_size())
- __annotate_increase(__str.__get_short_size() - __get_short_size());
+ if (__old_size < __str.__get_short_size())
+ __annotate_increase(__str.__get_short_size() - __old_size);
__rep_ = __str.__rep_;
if (__old_size > __get_short_size())
__annotate_shrink(__old_size);
@@ -3014,9 +3004,9 @@ template <class _CharT, class _Traits, class _Allocator>
_LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string<_CharT, _Traits, _Allocator>&
basic_string<_CharT, _Traits, _Allocator>::assign(const value_type* __s) {
_LIBCPP_ASSERT_NON_NULL(__s != nullptr, "string::assign received nullptr");
+ size_type __n = traits_type::length(__s);
return __builtin_constant_p(*__s)
- ? (__fits_in_sso(traits_type::length(__s)) ? __assign_short(__s, traits_type::length(__s))
- : __assign_external(__s, traits_type::length(__s)))
+ ? (__fits_in_sso(__n) ? __assign_short(__s, __n) : __assign_external(__s, __n))
: __assign_external(__s);
}
// append
@@ -3089,18 +3079,11 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::pu
}
if (__sz == __cap) {
__grow_by_without_replace(__cap, 1, __sz, __sz, 0);
- __annotate_increase(1);
__is_short = false; // the string is always long after __grow_by
- } else
- __annotate_increase(1);
- pointer __p = __get_pointer();
- if (__is_short) {
- __p = __get_short_pointer() + __sz;
- __set_short_size(__sz + 1);
- } else {
- __p = __get_long_pointer() + __sz;
- __set_long_size(__sz + 1);
}
+ __annotate_increase(1);
+ pointer __p = __is_short ? (__set_short_size(__sz + 1), __get_short_pointer() + __sz)
+ : (__set_long_size(__sz + 1), __get_long_pointer() + __sz);
traits_type::assign(*__p, __c);
traits_type::assign(*++__p, value_type());
}
@@ -3477,11 +3460,13 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
template <class _CharT, class _Traits, class _Allocator>
inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocator>::clear() _NOEXCEPT {
- size_type __old_size = size();
+ size_type __old_size;
if (__is_long()) {
+ __old_size = __get_long_size();
traits_type::assign(*__get_long_pointer(), value_type());
__set_long_size(0);
} else {
+ __old_size = __get_short_size();
traits_type::assign(*__get_short_pointer(), value_type());
__set_short_size(0);
}
@@ -3539,11 +3524,12 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
_LIBCPP_ASSERT_INTERNAL(__is_long(), "Trying to shrink small string");
// We're a long string and we're shrinking into the small buffer.
+ auto __ptr = __get_long_pointer();
+ auto __size = __get_long_size();
+ auto __cap = __get_long_cap();
+
if (__fits_in_sso(__target_capacity)) {
__annotation_guard __g(*this);
- auto __ptr = __get_long_pointer();
- auto __size = __get_long_size();
- auto __cap = __get_long_cap();
traits_type::copy(std::__to_address(__get_short_pointer()), std::__to_address(__ptr), __size + 1);
__set_short_size(__size);
__alloc_traits::deallocate(__alloc_, __ptr, __cap);
@@ -3554,21 +3540,19 @@ inline _LIBCPP_CONSTEXPR_SINCE_CXX20 void basic_string<_CharT, _Traits, _Allocat
try {
# endif // _LIBCPP_HAS_EXCEPTIONS
__annotation_guard __g(*this);
- auto __size = size();
auto __allocation = std::__allocate_at_least(__alloc_, __target_capacity + 1);
// The Standard mandates shrink_to_fit() does not increase the capacity.
// With equal capacity keep the existing buffer. This avoids extra work
// due to swapping the elements.
- if (__allocation.count - 1 > capacity()) {
+ if (__allocation.count > __cap) {
__alloc_traits::deallocate(__alloc_, __allocation.ptr, __allocation.count);
return;
}
__begin_lifetime(__allocation.ptr, __allocation.count);
- auto __ptr = __get_long_pointer();
traits_type::copy(std::__to_address(__allocation.ptr), std::__to_address(__ptr), __size + 1);
- __alloc_traits::deallocate(__alloc_, __ptr, __get_long_cap());
+ __alloc_traits::deallocate(__alloc_, __ptr, __cap);
__set_long_cap(__allocation.count);
__set_long_pointer(__allocation.ptr);
# if _LIBCPP_HAS_EXCEPTIONS
|
winner245
force-pushed
the
simplify-string
branch
2 times, most recently
from
345a0f2 to
aa89178
Compare
philnik777
requested changes
Feb 16, 2025
libcxx/include/string
Outdated
Comment on lines
3007
to
2975
| if (__builtin_constant_p(*__s)) { | ||
| const size_type __n = traits_type::length(__s); | ||
| return __fits_in_sso(__n) ? __assign_short(__s, __n) : __assign_external(__s, __n); | ||
| } | ||
| return __assign_external(__s); |
Contributor
There was a problem hiding this comment.
Suggested change
| if (__builtin_constant_p(*__s)) { | |
| const size_type __n = traits_type::length(__s); | |
| return __fits_in_sso(__n) ? __assign_short(__s, __n) : __assign_external(__s, __n); | |
| } | |
| return __assign_external(__s); | |
| if (auto __len = traits_type::length(__s); __builtin_constant_p(__len) && __fits_in_sso(__n)) | |
| return __assign_short(__s, __len); | |
| return __assign_external(__s); |
WDYT?
Contributor
Author
There was a problem hiding this comment.
Looks great—adopted! I have also double-checked that even with basic compiler optimization level (-O1), they yield the same code (https://godbolt.org/z/3jqM8cYEv), and the way you suggested is definitely the simplest.
winner245
force-pushed
the
simplify-string
branch
3 times, most recently
from
80f487c to
d897bac
Compare
philnik777
approved these changes
Feb 26, 2025
Contributor
philnik777
left a comment
philnik777
left a comment
There was a problem hiding this comment.
LGTM with nits addressed.
libcxx/include/string
Outdated
| basic_string<_CharT, _Traits, _Allocator>::__assign_no_alias(const value_type* __s, size_type __n) { | ||
| size_type __cap = __is_short ? static_cast<size_type>(__min_cap) : __get_long_cap(); | ||
| const auto __cap = __is_short ? static_cast<size_type>(__min_cap) : __get_long_cap(); | ||
| const auto __sz = __is_short ? __get_short_size() : __get_long_size(); |
Contributor
There was a problem hiding this comment.
Suggested change
| const auto __sz = __is_short ? __get_short_size() : __get_long_size(); | |
| const auto __size = __is_short ? __get_short_size() : __get_long_size(); |
Same throughout where you renamed variables anyways.
Comment on lines
2780
to
2784
| __set_long_size(1); | ||
| __p = __get_long_pointer(); |
Contributor
There was a problem hiding this comment.
Let's keep the same order here. It looks like you've used the original order in other places as well.
winner245
force-pushed
the
simplify-string
branch
from
d897bac to
ff05c97
Compare
winner245
force-pushed
the
simplify-string
branch
from
ff05c97 to
5d8cd96
Compare
Contributor
Author
|
Merging now with an approval and CIs green. |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/9469 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors
basic_stringa bit to simplify its implementation in the following ways:__get_short_size(),__get_long_size()), we call the general functions (size()) to hide the conditional checks and make the code more concise.{__set, __get}_long_pointerinstead of{__set, __get}_pointer().ifandelsebranches are now declared in a common scope to reduce redundancy.traits_type::length(__s), a variable is introduced to store its length. While modern compilers can optimize this with constant folding, explicitly storing the length improves code readability and makes the logic clearer.