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.

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

Implement P2438R2 string::substr() && #3057

Merged
merged 30 commits into from
Sep 1, 2022

Conversation

frederick-vs-ja
Copy link
Contributor

And make the old and new substr use default constructed allocators.

(And consistently refer to WG21-N4910 in <xstring>.)

Fixes #2928.
Fixes #3022.

As implementation details, the rvalue substring constructors reuse the source storage if and only if

  • the provided allocator and the allocator of the source compare equal, and
  • the constructed string doesn't fit into SSO buffer.

I have no idea how to portably test the probably intended behavior of newly added overloads...

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner August 26, 2022 03:56
@frederick-vs-ja
Copy link
Contributor Author

frederick-vs-ja commented Aug 26, 2022

It seems that EDG is buggy here:

  • EDG's __builtin_memcmp seemly doesn't work well with constexpr-dynamically-allocated storage,
  • EDG seemly mishandled overload resolution between T::operator const U& and T::operator U&&.

I'll skip relative cases for EDG.

@StephanTLavavej StephanTLavavej added the cxx23 C++23 feature label Aug 26, 2022
And conventionally use arrow comments.
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! I'll validate and push changes for the minor issues and nitpicks I found.

stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
tests/std/tests/P2438R2_substr_rvalue/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/P2438R2_substr_rvalue/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_003022_substr_allocator/test.cpp Outdated Show resolved Hide resolved
tests/std/tests/GH_003022_substr_allocator/test.cpp Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Aug 31, 2022
@StephanTLavavej
Copy link
Member

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

Copy link
Member

@CaseyCarter CaseyCarter left a comment

Choose a reason for hiding this comment

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

Going to approve because I don't want to reset testing, but I'll address these comments in a followup.

stl/inc/xstring Show resolved Hide resolved
stl/inc/xstring Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I've resolved a trivial adjacent-add conflict in tests/std/test.lst.

@StephanTLavavej StephanTLavavej merged commit 434f83c into microsoft:main Sep 1, 2022
@StephanTLavavej
Copy link
Member

Thanks for implementing this feature in one of the STL's most-used data structures! 😻 ✅ 🧶

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature
Projects
None yet
3 participants