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.

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

[flat.map][flat.multimap] Exposition-only formatting for c, comp, compare, and key-equiv #6404

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[flat.map][flat.multimap] Exposition-only formatting for c, comp, compare, and key-equiv #6404

wants to merge 3 commits into from

Conversation

frederick-vs-ja
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja commented Jul 23, 2023 •
edited

Also changes key_equiv to key-equiv.

I think it's OK to keep the member comp of key_equiv not marked \exposid, since the identifier comp is already reserved by priority_queue::comp. Implementations can use the name comp as-is or use another name.

key_equiv and value_compare's exposition-only constructors are less than ideal to me. But I think they should be handled in an LWG issue because additional copy of comparator seems to be observable. Edit: now this is LWG3959.

Follows up #5914. Fixes #5812.

JohelEGP
JohelEGP suggested changes Jul 23, 2023
View reviewed changes
source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 10, 2023

Thank you, this would be quite nice to land. @Dani-Hub, @CaseyCarter, would you perhaps be able to lend another pair of eyes?

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 10, 2023

@frederick-vs-ja Could you perhaps retrigger the precommit checks so we can get a downloadable PDF?

@Dani-Hub
Copy link
Member

Thank you, this would be quite nice to land. @Dani-Hub, @CaseyCarter, would you perhaps be able to lend another pair of eyes?

I would happily do so, but I'm away during from an 30 minutes from now until Tuesday and I'm unable to do that right now.

@frederick-vs-ja
Copy link
Contributor Author

@frederick-vs-ja Could you perhaps retrigger the precommit checks so we can get a downloadable PDF?

I've rebased and force-pushed, not sure whether this is sufficent...

@Dani-Hub
Copy link
Member

The changes look good to me, but I'd like to point out that similar fixes in e.g. for c and compare in [flat.multiset.modifiers] have not been applied. The issue says that it does only touch [flat.map] and [flat.multimap], so this is formally OK.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 14, 2023

@Dani-Hub Thanks! Yes, good point, we should fix those cases, too.

@frederick-vs-ja
Copy link
Contributor Author

@Dani-Hub @tkoeppe Sorry for noticing the comments so late. Now the changes are also applied to [flat.multiset.modifiers].

@Dani-Hub
Copy link
Member

@Dani-Hub @tkoeppe Sorry for noticing the comments so late. Now the changes are also applied to [flat.multiset.modifiers].

Looks good to me - Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dani-Hub Dani-Hub Dani-Hub approved these changes

@JohelEGP JohelEGP JohelEGP 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.

Style exposition-only members of flat_map, flat_multimap, flat_set, flat_multiset consistently
4 participants
@frederick-vs-ja @tkoeppe @Dani-Hub @JohelEGP