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

[flat.set.modifiers] (first, last) should be rg #6246

Merged
merged 1 commit into from May 9, 2023

Conversation

Quuxplusone
Copy link
Contributor

Restore consistency with [flat.map.modifiers]/13, which already uses ranges::distance(rg).

However, @jwakely can I also get some LWG guidance here? Right now, there is no insert_range in flat.multiset.modifiers, and there is no flat.multimap.modifiers at all. My intuition is that this insert_range wording should appear in both places.

I'm planning P2767R0 "flat_foo omnibus" for Varna. Should I submit a pull-request before Varna to add a section named [flat.multimap.modifiers], with contents similar to [flat.map.modifiers]? Should I just make P2767 introduce all of [flat.multimap.modifiers]? Or does LWG think it's "obvious" what flat_multimap::insert_range should do, and therefore it shouldn't be documented explicitly? See the index of library names for the status quo.

Restore consistency with [flat.map.modifiers]/13,
which already uses `ranges::distance(rg)`.
Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

Given that first, last are not even in-scope identifiers here, this seems good to me.

@jwakely ?

@jensmaurer jensmaurer requested a review from jwakely May 9, 2023 04:56
@jensmaurer
Copy link
Member

@Quuxplusone , if you write a paper introducing a section of new text, please don't post a pull request doing the same.

Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

The change is correct. This is just a copy&paste error in the incoming paper (P1222R4), reusing the text from the insert(InputIterator first, InputIterator last) function in paragraph 6.

@jwakely
Copy link
Member

jwakely commented May 9, 2023

Should I submit a pull-request before Varna to add a section named [flat.multimap.modifiers], with contents similar to [flat.map.modifiers]?

No, that would not be editorial.

@jwakely
Copy link
Member

jwakely commented May 9, 2023

Right now, there is no insert_range in flat.multiset.modifiers, and there is no flat.multimap.modifiers at all. My intuition is that this insert_range wording should appear in both places.

IIRC it's intentional that we don't repeat redundant wording:

Except as otherwise noted, operations on flat_multimap are equivalent to those of flat_map

Descriptions are provided here only for operations on flat_multiset that are not described in one of the
general sections or for operations where there is additional semantic information.

It would be better if we used similar wording for those two places, but I don't think we need to duplicate specifications if there is no difference. We have been doing something similar for unique_ptr<T> and unique_ptr<T[]> for 10+ years.

@Quuxplusone
Copy link
Contributor Author

Right now, there is no insert_range in flat.multiset.modifiers, and there is no flat.multimap.modifiers at all.

IIRC it's intentional that we don't repeat redundant wording:

[flat.multimap.overview/4] Except as otherwise noted, operations on flat_multimap are equivalent to those of flat_map, except that flat_multimap operations do not remove or replace elements with equal keys.
[Example 1: flat_multimap constructors and emplace do not erase non-unique elements after sorting them. —end example]

[flat.multiset.overview/4] Descriptions are provided here only for operations on flat_multiset that are not described in one of the general sections or for operations where there is additional semantic information.

It would be better if we used similar wording for those two places, but I don't think we need to duplicate specifications if there is no difference. We have been doing something similar for unique_ptr<T> and unique_ptr<T[]> for 10+ years.

Okay, that makes sense. I had missed that wording, and as you say, it's different in the two places. My paper will not propose to add [flat.multimap.modifiers], at least not for this reason.

flat_multiset's current wording is basically the same as the wording in every container's overview; e.g. [vector.overview]/2, [set.overview]/2, [multimap.overview]/2, etc:

Descriptions are provided here only for operations on xxx that are not described in one of those tables or for operations where there is additional semantic information.

So we must re-describe std::multimap::insert(P&&), because its Constraints element counts as "additional semantic information," even though its description matches that of std::map::insert(P&&). But, because of [flat.multimap.overview]/4, we don't feel the need to re-describe flat_multimap::insert(P&&) because its description (semantics and all) matches that of flat_map::insert(P&&).


My next question is, then, should I (submit a PR | propose in a paper) that [flat.multiset.overview/4] should be harmonized with [flat.multimap.overview/4]? For example perhaps they should both say:

Except as otherwise noted, operations on flat_multixxx are semantically equivalent to the corresponding operations flat_xxx, except that flat_multixxx operations do not remove or replace elements with equivalent keys.
[Example 1: flat_multixxx constructors do not remove duplicate elements after sorting. —end example]

I'm not sure how to express the idea that flat_multimap's "corresponding" member functions can take sorted_equivalent_t instead of sorted_unique_t.

Or how to express that the return value of flat_multimap::emplace derives from the Returns element of flat_map::emplace in the "obvious" way (i.e., just quietly drop the bool part of the pair), so it's okay that we don't re-specify it in [flat.multimap.modifiers]. (My paper will propose to drop the Constraint element from flat_map::emplace, btw, so please don't let that particular element of "semantic information" affect your reply.)


FYI, my paper-in-progress is D2767R0 "flat_map/flat_set omnibus." (I haven't completed all sections yet. Haven't even started some of them.) If you have suggestions on the format, please let me know here or by email. It'll be at least as messy to review as #5923, because it starts by repeating #5923's proposed editorial change and then goes on with additional less-editorial changes from there.

@jensmaurer jensmaurer merged commit 02545c7 into cplusplus:main May 9, 2023
2 checks passed
@Quuxplusone Quuxplusone deleted the flatmap-dist branch May 26, 2023 22:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants