Skip to content

[libc++] Implement std::flat_multiset #128363

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 23 commits into from
Apr 6, 2025
Merged

[libc++] Implement std::flat_multiset #128363

merged 23 commits into from
Apr 6, 2025

Conversation

huixie90
Copy link
Member

@huixie90 huixie90 commented Feb 22, 2025 •
edited
Loading

fixes #105193

@huixie90 huixie90 force-pushed the hxie/flat_multiset branch from da0281d to df09bee Compare March 9, 2025 18:25
@huixie90 huixie90 force-pushed the hxie/flat_multiset branch from 14b6ee7 to 437a643 Compare March 23, 2025 15:37
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 23, 2025 •
edited
Loading

✅ With the latest revision this PR passed the C/C++ code formatter.

@frederick-vs-ja frederick-vs-ja mentioned this pull request Mar 28, 2025
Merged
@huixie90 huixie90 marked this pull request as ready for review March 28, 2025 10:31
@huixie90 huixie90 requested a review from a team as a code owner March 28, 2025 10:31
@philnik777
Copy link
Contributor

You mean flat_multiset, right?

@ldionne ldionne changed the title [libc++] Implement std::multiset [libc++] Implement std::flat_multiset Mar 28, 2025
ldionne
ldionne approved these changes Mar 28, 2025
View reviewed changes
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I have some comments, but once they are addressed I am comfortable with this being merged. I did a less thorough review but I am relying on the fact that this is so similar to the other flat containers that most aspects of this have already been discussed many times.

libcxx/include/__flat_set/flat_multiset.h

if (!__prev_larger && !__next_smaller) [[likely]] {
// hint correct, just use exact hint iterators
} else if (__prev_larger && !__next_smaller) {
Copy link
Member

Choose a reason for hiding this comment

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

This behavior is not technically mandated by the standard. Let's add a libc++ specific test to pin down that behavior. I don't care strongly whether we provide that behavior, but if we "break" that in the future I'd at least like for us to notice and make an explicit decision about it.

We should also add a similar test for flat_multimap.

Copy link
Member Author

Choose a reason for hiding this comment

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

After reading the spec again, i think this behaviour is mandated. We do have tests that test this behaviour.

This paragraph says flat_multiset meets the requirements of associative containers (with some exceptions)
https://eel.is/c++draft/flat.multiset.overview#2

And associative container's requirement for emplace_hint is that the insertion location should be as close as the hint.
https://eel.is/c++draft/containers#associative.reqmts.general-58

@huixie90 huixie90 force-pushed the hxie/flat_multiset branch 2 times, most recently from 38296c9 to aeaa29e Compare March 29, 2025 18:45
@huixie90 huixie90 force-pushed the hxie/flat_multiset branch from ec2a310 to 1bcb3f6 Compare April 4, 2025 12:44
@huixie90 huixie90 merged commit 7013b51 into llvm:main Apr 6, 2025
81 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldionne ldionne ldionne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P1222R4: A Standard flat_set
3 participants
@huixie90 @philnik777 @ldionne