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

<xstring>: Use memmove in construction of basic_string when the source is a suitable contiguous range #4073

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Oct 6, 2023

Fixes #2901.

Criteria:

  • the source element type is integral
  • the target element type is an EcharT (encoded character type, i.e. char, wchar_t, char8_t, char16_t, char32_t)
  • the source and target element types are the same size
  • the target traits type is library-provided (thanks @CaseyCarter)
  • the source iterator is a contiguous iterator which doesn't dereference to a volatile lvalue.

Perhaps we can extend the criteria to move_iterator and unscoped enumeration types later.

Clang's __builtin_memcpy doesn't work when copying a char array to a char8_t array in constant evaluation (nor does GCC's), so this PR isn't using it in the involved construction.

The test P1206R7_string_from_range is largely extended to cover some related contiguous ranges and character types. The original test_c_array seemed to be a duplicate of test_copyable_views, so I changed it to properly cover built-in arrays.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner October 6, 2023 07:21
@frederick-vs-ja frederick-vs-ja changed the title <xstring>: Use memcpy in construction of basic_string when the source is a suitable contiguos range <xstring>: Use memcpy in construction of basic_string when the source is a suitable contiguous range Oct 6, 2023
@CaseyCarter CaseyCarter added the performance Must go faster label Oct 6, 2023
@StephanTLavavej
Copy link
Member

Perhaps we can extend the criteria to move_iterator and unscoped enumeration types later.

Such basic_string constructions would be extremely rare; I don't think it's worth spending maintenance complexity on that. Handling the u8string/string and similar scenarios here (like const unsigned short* to wstring) should be sufficient.

@StephanTLavavej
Copy link
Member

I have updated the PR description's criteria to match the code.

@StephanTLavavej

This comment was marked as resolved.

stl/inc/xstring Outdated Show resolved Hide resolved
stl/inc/xstring Outdated Show resolved Hide resolved
frederick-vs-ja and others added 2 commits October 7, 2023 23:53
Co-authored-by: Casey Carter <cartec69@gmail.com>
Co-authored-by: Casey Carter <cartec69@gmail.com>
@frederick-vs-ja frederick-vs-ja changed the title <xstring>: Use memcpy in construction of basic_string when the source is a suitable contiguous range <xstring>: Use memmove in construction of basic_string when the source is a suitable contiguous range Oct 9, 2023
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej

This comment was marked as resolved.

@frederick-vs-ja

This comment was marked as resolved.

@frederick-vs-ja
Copy link
Contributor Author

Oh, I'm sorry for force-pushing because I misconfigured the git settings.

@CaseyCarter CaseyCarter removed their assignment Oct 13, 2023
@StephanTLavavej StephanTLavavej self-assigned this Oct 13, 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 36a6a3e into microsoft:main Oct 14, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for improving the performance of one of the STL's most important types! 🚀 😸 🎉

@frederick-vs-ja frederick-vs-ja deleted the string-from-contiguous branch October 15, 2023 01:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<xstring>: conversion between basic_string specializations could sometimes memcpy
3 participants