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

Cleanups and enhancements for basic_string #3984

Merged
merged 6 commits into from
Sep 21, 2023
Merged

Cleanups and enhancements for basic_string #3984

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
acd0821
merge `_Memcpy_val_from`
achabense Aug 22, 2023
f7e8135
simplify construction from initializer_list
achabense Aug 22, 2023
6ed3396
improve exception guarantee
achabense Aug 22, 2023
3cda46d
improve usage of `_Calculate_growth`
achabense Aug 22, 2023
14b7558
noexcept enhancements
achabense Aug 22, 2023
a0716eb
nitpick
achabense Aug 22, 2023
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
75 changes: 38 additions & 37 deletions stl/inc/xstring
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2673,8 +2673,7 @@ private:
return;
}

_My_data._Myres = _Small_string_capacity;
size_type _New_capacity = _Calculate_growth(_Count);
size_type _New_capacity = _Calculate_growth(_Count, _Small_string_capacity, max_size());
const pointer _New_ptr = _Allocate_for_capacity(_Al, _New_capacity); // throws
_Construct_in_place(_My_data._Bx._Ptr, _New_ptr);

Expand Down Expand Up @@ -2731,7 +2730,7 @@ private:
}

_Elem* const _Old_ptr = _My_data._Myptr();
size_type _New_capacity = _Calculate_growth(_My_data._Mysize);
size_type _New_capacity = _Calculate_growth(_My_data._Mysize + 1);
Copy link
Contributor Author

@achabense achabense Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is not practically behavior-changing (won't affect resulting value), but I think it will be clearer to +1, as this is the only place that use _Mysize as input, and _Calculate_growth accept the first argument as _Requested, and this will be in line with push_back (using _Reallocate_grow_by(1, ...) which does _Calculate_growth(_My_data._Mysize + 1,...).

I'm neutral about +1 change, as it might slightly affect codegen; a comment about why to use _My_data._Mysize (just to do x1.5 evaluation) might be better.

Copy link
Member

Choose a reason for hiding this comment

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

I think + 1 makes sense here.

const pointer _New_ptr = _Allocate_for_capacity(_Al, _New_capacity); // throws

_Traits::copy(_Unfancy(_New_ptr), _Old_ptr, _My_data._Mysize);
Expand Down Expand Up @@ -3010,15 +3009,6 @@ public:
}

private:
void _Memcpy_val_from(const basic_string& _Right) noexcept {
_STL_INTERNAL_CHECK(_Can_memcpy_val);
const auto _My_data_mem =
reinterpret_cast<unsigned char*>(_STD addressof(_Mypair._Myval2)) + _Memcpy_val_offset;
const auto _Right_data_mem =
reinterpret_cast<const unsigned char*>(_STD addressof(_Right._Mypair._Myval2)) + _Memcpy_val_offset;
_CSTD memcpy(_My_data_mem, _Right_data_mem, _Memcpy_val_size);
}

_CONSTEXPR20 void _Take_contents(basic_string& _Right) noexcept {
// assign by stealing _Right's buffer
// pre: this != &_Right
Expand All @@ -3043,7 +3033,11 @@ private:
}
#endif // _ITERATOR_DEBUG_LEVEL != 0

_Memcpy_val_from(_Right);
const auto _My_data_mem =
reinterpret_cast<unsigned char*>(_STD addressof(_Mypair._Myval2)) + _Memcpy_val_offset;
const auto _Right_data_mem =
reinterpret_cast<const unsigned char*>(_STD addressof(_Right._Mypair._Myval2)) + _Memcpy_val_offset;
_CSTD memcpy(_My_data_mem, _Right_data_mem, _Memcpy_val_size);
_Right._Tidy_init();
return;
}
Expand Down Expand Up @@ -3075,12 +3069,12 @@ private:
const auto _Right_ptr = _Right_data._Myptr();
auto& _Al = _Getal();
if (_Allocators_equal(_Al, _Right._Getal()) && _Result_size > _Small_string_capacity) {
_Mypair._Myval2._Alloc_proxy(_GET_PROXY_ALLOCATOR(_Alty, _Al));

if (_Roff != 0) {
_Traits::move(_Right_ptr, _Right_ptr + _Roff, _Result_size);
}
_Right._Eos(_Result_size);

_Mypair._Myval2._Alloc_proxy(_GET_PROXY_ALLOCATOR(_Alty, _Al));
_Take_contents(_Right);
} else {
_Construct<_Construct_strategy::_From_ptr>(_Right_ptr + _Roff, _Result_size);
Expand All @@ -3091,11 +3085,7 @@ private:
public:
_CONSTEXPR20 basic_string(initializer_list<_Elem> _Ilist, const _Alloc& _Al = allocator_type())
: _Mypair(_One_then_variadic_args_t{}, _Al) {
auto&& _Alproxy = _GET_PROXY_ALLOCATOR(_Alty, _Getal());
_Container_proxy_ptr<_Alty> _Proxy(_Alproxy, _Mypair._Myval2);
_Tidy_init();
assign(_Ilist.begin(), _Convert_size<size_type>(_Ilist.size()));
_Proxy._Release();
_Construct<_Construct_strategy::_From_ptr>(_Ilist.begin(), _Convert_size<size_type>(_Ilist.size()));
}

_CONSTEXPR20 basic_string& operator=(initializer_list<_Elem> _Ilist) {
Expand Down Expand Up @@ -4274,9 +4264,6 @@ public:
auto& _My_data = _Mypair._Myval2;
auto& _Right_data = _Right._Mypair._Myval2;

const bool _My_large = _My_data._Large_mode_engaged();
const bool _Right_large = _Right_data._Large_mode_engaged();

Copy link
Contributor Author

@achabense achabense Aug 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

STL/stl/inc/xstring

Line 4272 in 60f1885

using _STD swap;

Is it encouraged to move this using _STD swap into the if block where it is used? I'm not sure as I find using _STD swap is usually defined at the beginning of functions.

Copy link
Member

Choose a reason for hiding this comment

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

Making it as local as possible seems like an improvement.

#if !defined(_INSERT_STRING_ANNOTATION)
if constexpr (_Can_memcpy_val) {
#if _HAS_CXX20
Expand All @@ -4297,6 +4284,9 @@ public:
}
#endif // !defined(_INSERT_STRING_ANNOTATION)

const bool _My_large = _My_data._Large_mode_engaged();
const bool _Right_large = _Right_data._Large_mode_engaged();

if (_My_large && _Right_large) { // swap buffers, iterators preserved
swap(_My_data._Bx._Ptr, _Right_data._Bx._Ptr); // intentional ADL
} else if (_My_large) { // swap large with small
Expand Down Expand Up @@ -4801,7 +4791,7 @@ private:
return *this;
}

_CONSTEXPR20 void _Become_small() {
_CONSTEXPR20 void _Become_small() noexcept {
// release any held storage and return to small string mode
auto& _My_data = _Mypair._Myval2;
_STL_INTERNAL_CHECK(_My_data._Large_mode_engaged());
Expand Down Expand Up @@ -5021,7 +5011,7 @@ _NODISCARD _CONSTEXPR20 bool operator==(

_EXPORT_STD template <class _Elem, class _Traits, class _Alloc>
_NODISCARD _CONSTEXPR20 bool operator==(
const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) {
const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) noexcept /* strengthened */ {
return _Left._Equal(_Right);
}

Expand All @@ -5034,12 +5024,13 @@ _NODISCARD constexpr _Get_comparison_category_t<_Traits> operator<=>(

_EXPORT_STD template <class _Elem, class _Traits, class _Alloc>
_NODISCARD constexpr _Get_comparison_category_t<_Traits> operator<=>(
const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) {
const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) noexcept /* strengthened */ {
return static_cast<_Get_comparison_category_t<_Traits>>(_Left.compare(_Right) <=> 0);
}
#else // ^^^ _HAS_CXX20 / !_HAS_CXX20 vvv
template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator==(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) {
_NODISCARD bool operator==(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) noexcept
/* strengthened */ {
return _Right._Equal(_Left);
}

Expand All @@ -5050,12 +5041,14 @@ _NODISCARD bool operator!=(
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator!=(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) {
_NODISCARD bool operator!=(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) noexcept
/* strengthened */ {
return !(_Left == _Right);
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator!=(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) {
_NODISCARD bool operator!=(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) noexcept
/* strengthened */ {
return !(_Left == _Right);
}

Expand All @@ -5066,12 +5059,14 @@ _NODISCARD bool operator<(
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator<(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) {
_NODISCARD bool operator<(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) noexcept
/* strengthened */ {
return _Right.compare(_Left) > 0;
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator<(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) {
_NODISCARD bool operator<(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) noexcept
/* strengthened */ {
return _Left.compare(_Right) < 0;
}

Expand All @@ -5082,12 +5077,14 @@ _NODISCARD bool operator>(
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator>(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) {
_NODISCARD bool operator>(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) noexcept
/* strengthened */ {
return _Right < _Left;
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator>(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) {
_NODISCARD bool operator>(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) noexcept
/* strengthened */ {
return _Right < _Left;
}

Expand All @@ -5098,12 +5095,14 @@ _NODISCARD bool operator<=(
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator<=(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) {
_NODISCARD bool operator<=(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) noexcept
/* strengthened */ {
return !(_Right < _Left);
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator<=(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) {
_NODISCARD bool operator<=(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) noexcept
/* strengthened */ {
return !(_Right < _Left);
}

Expand All @@ -5114,12 +5113,14 @@ _NODISCARD bool operator>=(
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator>=(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) {
_NODISCARD bool operator>=(_In_z_ const _Elem* const _Left, const basic_string<_Elem, _Traits, _Alloc>& _Right) noexcept
/* strengthened */ {
return !(_Left < _Right);
}

template <class _Elem, class _Traits, class _Alloc>
_NODISCARD bool operator>=(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) {
_NODISCARD bool operator>=(const basic_string<_Elem, _Traits, _Alloc>& _Left, _In_z_ const _Elem* const _Right) noexcept
/* strengthened */ {
return !(_Left < _Right);
}
#endif // ^^^ !_HAS_CXX20 ^^^
Expand Down