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

<list>, <string>: Remove allocator-moving tagged internal constructors #4976

Merged

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Sep 22, 2024

Fixes #4929.

Currently, these internal tagged constructors are implemented in some non-conforming manner such that they can be selected by conforming user codes. They exist for constructing containers with the stored allocators move-constructed, while the corresponding standard constructors copy-construct allocators. This PR removes the these internal tagged constructors and makes allocators copy-constructed.

Although the Standard currently requires that sometimes the allocator is move-constructed ([container.reqmts]), it's highly unusual and perhaps pointless to make move construction of an allocator type not equivalent to copy construction, because LWG-2593 requires that the move construction doesn't change the value of the source allocator. It seems that there can be different side effects can be different, but such difference can't affect further allocations.

In the future, there might be an LWG issue to allow move construction of containers to copy-construct allocators.

Affected tests - skipped during the development, unskipped later due to another approach

libcxx:

  • std/containers/unord/unord.map/unord.map.cnstr/move.pass.cpp
  • std/containers/unord/unord.multimap/unord.multimap.cnstr/move.pass.cpp
  • std/containers/unord/unord.multiset/unord.multiset.cnstr/move.pass.cpp
  • std/containers/unord/unord.set/unord.set.cnstr/move.pass.cpp

MSVC STL:

  • VSO_0102478_moving_allocators

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner September 22, 2024 16:53
@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 23, 2024
@StephanTLavavej StephanTLavavej self-assigned this Sep 23, 2024
@StephanTLavavej
Copy link
Member

Thanks! 😻 I fixed the basic EH guarantee and a couple of test nitpicks - please double-check.

@StephanTLavavej StephanTLavavej removed their assignment Sep 27, 2024
@StephanTLavavej StephanTLavavej self-assigned this Sep 27, 2024
@StephanTLavavej

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej removed their assignment Sep 27, 2024
@StephanTLavavej StephanTLavavej self-assigned this Sep 27, 2024
@StephanTLavavej
Copy link
Member

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

@StephanTLavavej
Copy link
Member

I had to push an additional commit due to a /clr:pure bug that rejects the test:

D:\msvc\src\vctools\crt\github\tests\std\tests\GH_004929_internal_tag_constructors\test.cpp(27): error C2075: '$S1': initialization requires a brace-enclosed initializer list

I was too much of a lazy kitty for a targeted workaround, so I used the impure matrix.

@StephanTLavavej StephanTLavavej merged commit 1c3e0d6 into microsoft:main Sep 28, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for making is_constructible_v report the right answers here! 🚧 👷 🏗️

@frederick-vs-ja frederick-vs-ja deleted the copy-construct-allocator branch September 29, 2024 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

<string>, <sstream>: Is the _String_constructor_rvalue_allocator_tag internal constructor necessary?
2 participants