Logic cleanups for `basic_string` #3862

achabense · 2023-07-11T13:53:31Z

This is a reorganized version of #3830.

Various logic cleanups to make basic_string easier to maintain.
Fixes a bug in operator=(const&) which will break invariant.

Thanks to @StephanTLavavej, @fsb4000 and @frederick-vs-ja for review in the original pr!

stl/inc/xstring

achabense · 2023-07-11T13:56:10Z

stl/inc/xstring

+                const _Elem* const _Right_ptr = _Right._Mypair._Myval2._Myptr();
+                if (_Entails_large_string(_Right_size)) {
+                    const auto _New_capacity =
+                        _Calculate_growth(_Right_size, _SMALL_STRING_CAPACITY, _Right.max_size());


[won't be included in this pr]
If this fix is adopted, the second parameter of _Calculate_growth(x,y,z) will be uniformly _SMALL_STRING_CAPACITY and further rectification will be possible.
There is also a strange use of _Calculate_growth(newsize) in _Construct_from_iter. According to impl logic it is performing capacity*=1.5 logic. What about replacing the grow logic here with _Reallocate_grow_by or push_back?

STL/stl/inc/xstring

Line 2712 in 40640c6

const size_type _New_capacity = _Calculate_growth(_My_data._Mysize);

achabense · 2023-07-11T13:57:31Z

stl/inc/xstring

+    // invariant: _Myres >= _Mysize, and _Myres >= _SMALL_STRING_CAPACITY (after string's construction)
+    // both _Mysize and _Myres doesn't take account of the extra null terminator
+    size_type _Mysize = 0; // current length of string (size)
+    size_type _Myres  = 0; // current storage reserved for string (capacity)


[won't be included in this pr]
suggestion: we can make _Myres initialized to _SMALL_STRING_CAPACITY to make _Myres >= _SMALL_STRING_CAPACITY throughout the lifetime of _String_val.

stl/inc/xstring

…d` logic with it

StephanTLavavej · 2023-07-28T02:14:22Z

As a special exception to our usual way of working, I've force-pushed the branch to rebase it on main and linearize the history. I verified (with git diff) that this results in the exact same state. I fused commits (e.g. _NODISCARD) that were logically grouped together, ironed out commits that were purely addressing earlier mistakes, and decomposed a couple of commits into finer-grained ones (e.g. auto, _CONSTEXPR20).

THANK YOU for the fine-grained commits, they were an enormous help in reviewing. After linearization, I was able to see the correctness of each transformation and I have high confidence in the resulting state. Overall I think that this is a great cleanup and makes the complicated basic_string code more robust for future maintenance - particularly avoiding off-by-one errors around the SSO, and extracting the allocate_at_least and start-element-lifetimes complexity that has recently accreted.

Finally, I added commits to address the minor issues I found while reviewing:

Serially run the tests in Dev09_181509_tr1_inf_loop_uniform_int_ull.
- This should avoid the mysterious compiler back-end ICE when cross-compiling from x64 to ARM. This test originally (6+ years ago) tested so many cases, it was reasonable to run them in parallel. Those cases got chopped away over the years (last major change was in 2017), and the resulting test runs in ~4 seconds serially. Because we test so many configurations in parallel, there is no advantage to making this test run parallel inside itself, so I've converted it to the C++14 matrix, dropped the use of parallel for_each(), and removed the /clr workaround (for crashes with parallel algorithms).
Fix comment grammar.
In _Deallocate_for_capacity, pass _Old_ptr by value.
- We (and the Standard) conventionally pass potentially-fancy pointers by value; they are expected to be cheap to copy.
Drop redundant public: after the removal of _Copy_assign_val_from_small.
Add test_string_copy_assign_pocca_sso().
- I verified that this fails before your fix, and passes afterwards. The existing coverage didn't exercise the SSO transition enough. It was simplest to just add a new case rather than trying to alter the existing coverage, and this test conveniently had the POCCA logic ready to go.
Restore shrink_to_fit to allocate exactly.
- We should behave like vector and avoid the allocate_at_least optimization here. Let's avoid surprising users; they already worry about shrink_to_fit's non-binding nature.

stl/inc/xstring

achabense · 2023-07-28T11:46:17Z

stl/inc/xstring

            _Become_small();
            return;
        }

-        const size_type _Target_capacity = (_STD min)(_My_data._Mysize | _ALLOC_MASK, max_size());
+        size_type _Target_capacity = (_STD min)(_My_data._Mysize | _ALLOC_MASK, max_size());


(no changes requested) As _Exactly mode doesn't really require input capacity to be a referrence, I think we can restore constness for _Target_capacity in the future. Also here in <vector>:

STL/stl/inc/vector

Lines 1705 to 1706 in 0fe653a

size_type _Newcapacity = static_cast<size_type>(_Oldlast - _Oldfirst);

_Reallocate<_Reallocation_policy::_Exactly>(_Newcapacity);

CaseyCarter

I'll verify and push a change to address my comments, which are mostly naming nits.

stl/inc/xstring

Rename `_ALLOC_MASK`, `_SMALL_STRING_CAPACITY` and `_LEAST_ALLOCATION_SIZE` to avoid the macro identifier namespace. Comment that `_BUF_SIZE` is used by the debugger visualizer (and hence shouldn't be renamed).

... to avoid confusion about size vs. capacity since `_Large_string_engaged()` can be `true` for a very small string with a large capacity.

With the addition of `_SMALL_STRING_CAPACITY`, I think the expressions themselves (e.g., `_Count > _SMALL_STRING_CAPACITY`) are more readable than calls to the function.

It's more clearly the opposite of "large mode".

... to the more descriptive `_Actual_allocation_size`.

Grepping is quick, and this is very likely to bitrot.

StephanTLavavej · 2023-08-10T01:08:50Z

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

StephanTLavavej · 2023-08-11T02:21:18Z

Thanks again for this maintainability overhaul and runtime correctness fix! 😻 🪄 🚀

achabense requested a review from a team as a code owner July 11, 2023 13:53

achabense commented Jul 11, 2023

View reviewed changes

stl/inc/xstring Outdated Show resolved Hide resolved

StephanTLavavej added the enhancement Something can be improved label Jul 12, 2023

StephanTLavavej mentioned this pull request Jul 12, 2023

Toolset update: VS 2022 17.7 Preview 3, Clang 16.0.5 #3866

Merged

achabense commented Jul 12, 2023

View reviewed changes

stl/inc/xstring Outdated Show resolved Hide resolved

achabense commented Jul 12, 2023

View reviewed changes

stl/inc/xstring Show resolved Hide resolved

achabense commented Jul 12, 2023

View reviewed changes

stl/inc/xstring Outdated Show resolved Hide resolved

StephanTLavavej self-assigned this Jul 12, 2023

This comment was marked as resolved.

Sign in to view

StephanTLavavej removed their assignment Jul 19, 2023

This comment was marked as resolved.

Sign in to view

achabense commented Jul 19, 2023

View reviewed changes

stl/inc/xstring Outdated Show resolved Hide resolved

stl/inc/xstring Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

StephanTLavavej self-assigned this Jul 22, 2023

achabense added 9 commits July 27, 2023 16:02

Make string(const T*) and string(const T*, const Alloc&) adjacent

45e738d

Add _NODISCARD for _String_val methods

9f726f9

make _Activate_SSO_buffer _CONSTEXPR20

02b90ed

Make types of _BUF_SIZE and _ALLOC_MASK clear

ca0953a

Introduce _SMALL_STRING_CAPACITY; replace _BUF_SIZE - 1 with it

ef708ed

Refine _Large_string_engaged; find a missed usecase for it

d3f736b

Introduce _Entails_large_string; replace wild `_Large_string_engage…

e1ce499

…d` logic with it

Replace capacity-decision logic with _Entails_large_string

e050101

Refine documentation of interaction part with basic_stringbuf

2b966c6

StephanTLavavej force-pushed the _String_logic_cleanups branch from 1d7d1f3 to 9b0e590 Compare July 28, 2023 01:51

StephanTLavavej removed their assignment Jul 28, 2023

StephanTLavavej approved these changes Jul 28, 2023

View reviewed changes

achabense commented Jul 28, 2023

View reviewed changes

stl/inc/xstring Outdated Show resolved Hide resolved

_Reallocation_policy => _Allocation_policy

3a27c90

StephanTLavavej approved these changes Jul 28, 2023

View reviewed changes

achabense commented Jul 28, 2023

View reviewed changes

StephanTLavavej assigned CaseyCarter Aug 2, 2023

CaseyCarter approved these changes Aug 9, 2023

View reviewed changes

CaseyCarter removed their assignment Aug 9, 2023

CaseyCarter added 6 commits August 9, 2023 13:23

Rename to avoid macro identifier space

6ea7f29

Rename `_ALLOC_MASK`, `_SMALL_STRING_CAPACITY` and `_LEAST_ALLOCATION_SIZE` to avoid the macro identifier namespace. Comment that `_BUF_SIZE` is used by the debugger visualizer (and hence shouldn't be renamed).

Rename _Large_string_engaged to _Large_mode_engaged

a2cb72b

... to avoid confusion about size vs. capacity since `_Large_string_engaged()` can be `true` for a very small string with a large capacity.

Expand _Entails_large_string into callsites

9921deb

With the addition of `_SMALL_STRING_CAPACITY`, I think the expressions themselves (e.g., `_Count > _SMALL_STRING_CAPACITY`) are more readable than calls to the function.

"small mode" instead of "SSO mode"

84b8ced

It's more clearly the opposite of "large mode".

Rename _Buf_res and _Released_buffer::_Res

9ed46dc

... to the more descriptive `_Actual_allocation_size`.

Don't enumerate users of _Least_allocation_size

e2ac4ce

Grepping is quick, and this is very likely to bitrot.

CaseyCarter force-pushed the _String_logic_cleanups branch from 231682b to e2ac4ce Compare August 9, 2023 20:35

StephanTLavavej approved these changes Aug 9, 2023

View reviewed changes

StephanTLavavej self-assigned this Aug 10, 2023

StephanTLavavej merged commit 29fc1c9 into microsoft:main Aug 11, 2023
35 checks passed

achabense deleted the _String_logic_cleanups branch August 11, 2023 02:39

StephanTLavavej mentioned this pull request Aug 11, 2023

libcxx sporadic failure in std/strings/basic.string/string.cons/copy.pass.cpp #3949

Closed

achabense mentioned this pull request Aug 28, 2023

<xstring>: Fix basic_string::operator=(const&) thoroughly #3997

Closed

AlexGuteniev mentioned this pull request Mar 12, 2024

<xstring>: constexpr string does not compile with -d1initall #4473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logic cleanups for `basic_string` #3862

Logic cleanups for `basic_string` #3862

achabense commented Jul 11, 2023 •

edited

Loading

achabense Jul 11, 2023 •

edited

Loading

achabense Jul 11, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

StephanTLavavej commented Jul 28, 2023

achabense Jul 28, 2023

CaseyCarter left a comment

StephanTLavavej commented Aug 10, 2023

StephanTLavavej commented Aug 11, 2023

	size_type _Newcapacity = static_cast<size_type>(_Oldlast - _Oldfirst);
	_Reallocate<_Reallocation_policy::_Exactly>(_Newcapacity);

Logic cleanups for basic_string #3862

Logic cleanups for basic_string #3862

Conversation

achabense commented Jul 11, 2023 • edited Loading

achabense Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

achabense Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

StephanTLavavej commented Jul 28, 2023

achabense Jul 28, 2023

Choose a reason for hiding this comment

CaseyCarter left a comment

Choose a reason for hiding this comment

StephanTLavavej commented Aug 10, 2023

StephanTLavavej commented Aug 11, 2023

Logic cleanups for `basic_string` #3862

Logic cleanups for `basic_string` #3862

achabense commented Jul 11, 2023 •

edited

Loading

achabense Jul 11, 2023 •

edited

Loading

achabense Jul 11, 2023 •

edited

Loading