Skip to content

P1679 string contains #1478

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

Merged
merged 8 commits into from
Jun 29, 2021
Merged

Conversation

WimLeflere
Copy link
Contributor

@WimLeflere WimLeflere commented Nov 17, 2020

C++23 string contains implementation

Paper: https://wg21.link/P1679R3
Draft issue: cplusplus/draft#4328
Draft PR: cplusplus/draft#4373

@WimLeflere WimLeflere requested a review from a team as a code owner November 17, 2020 19:52
@mnatsuhara
Copy link
Contributor

Thank you for your work in this PR! However, I'd like to note that we won't be able to review this PR for a while, as the relevant issue #1450 says:

Note: We're still working on finishing C++20. Until we're done with that (and the compiler implements distinct /std:c++20 and /std:c++latest options), we won't be able to review PRs for C++23 features.

@CaseyCarter CaseyCarter added the cxx23 C++23 feature label Nov 17, 2020
@StephanTLavavej StephanTLavavej linked an issue Nov 17, 2020 that may be closed by this pull request
@StephanTLavavej StephanTLavavej added the blocked Something is preventing work on this label Nov 17, 2020
@WimLeflere
Copy link
Contributor Author

My implementation is based on the starts_with/ends_with implementation.

I was wondering why the const char* function overload is noexcept.
constexpr bool ends_with(const _Elem* const _Right) const noexcept /* strengthened */

While this is explicitly not the case in the standard.

@StephanTLavavej
Copy link
Member

The Standard historically avoided marking non-throwing but precondition-ful functions as noexcept. (The idea was that certain implementations might want to detect and report precondition violations by throwing an exception.) We often "strengthen" such functions to be noexcept in our implementation, with a comment (this is allowed by the Standard). Recently, the Library Working Group has changed its mind about this convention, although the entire Standard hasn't been updated accordingly yet.

@SuperWig
Copy link
Contributor

Could be worth adding the explanation of the noexcept strengthening in the wiki.

Base automatically changed from master to main January 28, 2021 00:35
@StephanTLavavej StephanTLavavej removed the blocked Something is preventing work on this label Jun 2, 2021
@StephanTLavavej

This comment has been minimized.

# Conflicts:
#	stl/inc/xstring
#	stl/inc/yvals_core.h
@SuperWig
Copy link
Contributor

Seems like @StephanTLavavej missed one

This PR needs to merge main and resolve a conflict in yvals_core.h.

# Conflicts:
#	stl/inc/yvals_core.h
@StephanTLavavej StephanTLavavej self-assigned this Jun 16, 2021
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! My apologies for the delay in reviewing your PR (due in part to C++20 marathon exhaustion and summer vacations). All I found were minor issues, so to save time I'll push changes (mainly adding constexpr for the basic_string overloads and updating a test accordingly) and move this to Final Review. After a second maintainer can double-check everything, we'll able to merge this for VS 2022 17.0 Preview 3.

@StephanTLavavej StephanTLavavej removed their assignment Jun 23, 2021
@StephanTLavavej StephanTLavavej self-assigned this Jun 24, 2021
@StephanTLavavej StephanTLavavej merged commit 2682721 into microsoft:main Jun 29, 2021
@StephanTLavavej
Copy link
Member

Thanks for adding this feature to the Standard and implementing it here - and congratulations on your first microsoft/STL commit! 😸 🎉 🚀 This will ship in VS 2022 17.0 Preview 3.

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
Development

Successfully merging this pull request may close these issues.

P1679R3 contains() For basic_string/basic_string_view
6 participants