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

Jump to bottom

Cleanups and enhancements for basic_string #3984

Merged
merged 6 commits into from Sep 21, 2023
Merged

Cleanups and enhancements for basic_string #3984

merged 6 commits into from Sep 21, 2023

Conversation

achabense
Copy link
Contributor

  • Merge _Memcpy_val_from to its only caller.
  • Simplify basic_string(initializer_list)
  • Enhance exception guarantee for basic_string(&&, off). The rvalue will be unchanged if exception is thrown from _Alloc_proxy().
  • noexcept enhancements, mainly for comparisons with c-string.
  • Some other nitpicks.

@achabense achabense requested a review from a team as a code owner August 22, 2023 14:06
@github-actions github-actions bot added this to Initial Review in Code Reviews Aug 22, 2023
achabense
achabense commented Aug 22, 2023
View reviewed changes
stl/inc/xstring
@@ -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

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.

@achabense
Copy link
Contributor Author

achabense commented Aug 22, 2023 •
edited

  • STL/stl/inc/xstring

    Lines 2785 to 2786 in 60f1885

    _CONSTEXPR20 basic_string(basic_string&& _Right, const _Alloc& _Al) noexcept(
    _Alty_traits::is_always_equal::value) // strengthened

    I think this noexcept enhancement is not suitable, as it is not mandated by the standard, and involves _Alloc_proxy.
  • Also in

    STL/stl/inc/vector

    Lines 703 to 704 in 60f1885

    _CONSTEXPR20 vector(vector&& _Right, const _Identity_t<_Alloc>& _Al_) noexcept(
    _Alty_traits::is_always_equal::value) // strengthened
  • For some other noexcept functions like basic_string(&&), basic_string(al), I think we need to comment that the noexcept is mandated by the standard even if the functions all invlove _Alloc_proxy.

achabense
achabense commented Aug 22, 2023
View reviewed changes
stl/inc/xstring
@@ -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

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.

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Aug 24, 2023
@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Aug 29, 2023 •
edited

  • I think this noexcept enhancement is not suitable, as it is not mandated by the standard, and involves _Alloc_proxy.

Not sure whether we should remove the strengthening, since noexcept is good in release mode.

  • For some other noexcept functions like basic_string(&&), basic_string(al), I think we need to comment that the noexcept is mandated by the standard even if the functions all invlove _Alloc_proxy.

See #1035. I think we should say // TRANISITION, ABI, GH-1035, possibly terminates when _ITERATOR_DEBUG_LEVEL > 0 in the lines calling _Alloc_proxy.

@StephanTLavavej StephanTLavavej self-assigned this Sep 2, 2023
StephanTLavavej
StephanTLavavej approved these changes Sep 19, 2023
View reviewed changes
stl/inc/xstring
@@ -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
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.

stl/inc/xstring
@@ -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
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.

@StephanTLavavej
Copy link
Member

All of these cleanups look great - thanks!

@StephanTLavavej StephanTLavavej removed their assignment Sep 19, 2023
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in Code Reviews Sep 19, 2023
@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Sep 20, 2023
@StephanTLavavej
Copy link
Member

I have enough confidence in these cleanups to merge them without a second maintainer approval.

@StephanTLavavej StephanTLavavej self-assigned this Sep 20, 2023
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej merged commit 6f31505 into microsoft:main Sep 21, 2023
37 checks passed
Code Reviews automation moved this from Ready To Merge to Done Sep 21, 2023
@StephanTLavavej
Copy link
Member

🧵 🧶 🧹

@achabense achabense deleted the _StringCleanups branch September 22, 2023 03:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
enhancement Something can be improved
Projects
No open projects
Code Reviews
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@achabense @frederick-vs-ja @StephanTLavavej @CaseyCarter