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

Fix and extend key extraction for unique map/set containers #5050

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

frederick-vs-ja
Copy link
Contributor

Currently, the key-extracting optimization mechanism incorrectly catches some volatile-qualified glvalues (due to _Remove_cvref_t) and attempts to bind them to a const K& reference, which makes the program ill-formed. This PR make the mechanism never catch volatile glvalues, which makes volatile accepted under some conditions as specified by the standard.

In piecewise construction, when the first tuple argument is a 1-tuple, it's sometimes possible to directly treat its only tuple element as the key. Also, since C++23, it should be possible to directly get the key from suitable tuple<K, V> and array<K, 2> without additional construction. This PR attempts to extend the optimization mechanism to catch these cases.

The mechanism should also be usable for flat_set and flat_map.

Note that subrange is intentionally ignored because the corresponding get function copies (or moves) the key, which doesn't seem desired.

@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner October 28, 2024 16:59
@frederick-vs-ja frederick-vs-ja changed the title Fix and extend key extraction for unique map containers Fix and extend key extraction for unique map/set containers Oct 28, 2024
@StephanTLavavej

This comment was marked as outdated.

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Oct 28, 2024
@StephanTLavavej StephanTLavavej self-assigned this Oct 28, 2024
@CaseyCarter

This comment was marked as resolved.

stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
stl/inc/xmemory Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Oct 29, 2024
@StephanTLavavej
Copy link
Member

Thanks! 😸 I pushed some minor refactorings.

@StephanTLavavej
Copy link
Member

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

@StephanTLavavej StephanTLavavej added the performance Must go faster label Oct 30, 2024
@StephanTLavavej StephanTLavavej merged commit 41e3f51 into microsoft:main Oct 30, 2024
39 checks passed
@StephanTLavavej
Copy link
Member

Thanks for noticing and fixing this! 🦅 👁️ 🛠️

We usually changelog bugfixes as higher priority than perf improvements, but in this case volatile is extremely obscure while tuple and array aren't, so I'm going to changelog this as being primarily a perf improvement.

@frederick-vs-ja frederick-vs-ja deleted the extract-key branch October 30, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working performance Must go faster
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants