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

P0429R9 A Standard flat_map #5626

Merged
merged 3 commits into from Aug 14, 2022
Merged

P0429R9 A Standard flat_map #5626

merged 3 commits into from Aug 14, 2022

Conversation

jensmaurer
Copy link
Member

  • Replaced "may" with "can" in notes.
  • Replace && in constraints with prose "and" to avoid overfull \hbox

Fixes #5593
Fixes cplusplus/papers#101

@jensmaurer jensmaurer added this to the post-2022-07 milestone Jul 16, 2022
Copy link
Contributor

@JohelEGP JohelEGP left a comment

Choose a reason for hiding this comment

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

Is [assoc.format] coming from a different paper? There's an instruction in the paper to change it.

source/lib-intro.tex Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
source/containers.tex 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
Comment on lines +14462 to +14466
\pnum
Any sequence container\iref{sequence.reqmts} \tcode{C}
supporting \oldconcept{RandomAccessIterator} can be used
Copy link
Contributor

Choose a reason for hiding this comment

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

This must be referring to C's associated iterator type(s). How can this be improved? For reference:

Any sequence container with random access iterator and supporting operations front(), push_­back() and pop_­back() can be used to instantiate priority_­queue.
-- https://eel.is/c++draft/priority.queue#priqueue.overview-1.sentence-1

Copy link
Contributor

Choose a reason for hiding this comment

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

@jwakely, @CaseyCarter: Any thoughts? I don't think this is broken enough to warrant an immediate fix here, but I'm happy to entertain editorial improvement requests.

source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
@CaseyCarter
Copy link
Contributor

Is [assoc.format] coming from a different paper? There's an instruction in the paper to change it.

Yes, there's a mess of interactions here. P2286 adds [assoc.format], the flat_meow papers modify it, and p2585 stomps all over those (now unnecessary) edits. (The editorial instruction in P2585 "Change the the wording for associative containers as follows" could be a lot better.)

@jwakely
Copy link
Member

jwakely commented Jul 16, 2022

The editorial instruction in P2585 "Change the the wording for associative containers as follows" could be a lot better.

See the improved instructions in p2585r1, which is the version being polled at plenary.

source/containers.tex Outdated Show resolved Hide resolved
-> flat_map<@\exposid{iter-key-type}@<InputIterator>, @\exposid{iter-mapped-type}@<InputIterator>, Compare>;

template<ranges::input_range R, class Compare = less<@\exposid{range-key-type}@<R>>,
class Allocator>
Copy link
Contributor

@JohelEGP JohelEGP Jul 16, 2022

Choose a reason for hiding this comment

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

Is this right? What does it mean that the previous template parameter Compare is defaulted, but Allocator isn't, even though both corresponding function parameters are defaulted? Seems like you can't get this to work without defaulting this Allocator: https://godbolt.org/z/fYesrsbMz.

Suggested change
class Allocator>
class Allocator = allocator<ranges::range_value_t<R>>>
Suggested change
class Allocator>
class Allocator = allocator<@\exposid{range-mapped-type}@<R>>>

I think both suggestions fulfill the corresponding Constraints element.

Suggested change
class Allocator>
class Allocator = allocator<void>>

Copy link
Member

Choose a reason for hiding this comment

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

I think allocator<void> would be just as good. It's going to be rebound anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jwakely: are you (= LWG) happy to add a default argument allocator<void> here?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that's editorial. Needs a library issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #5800.

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
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
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
@JohelEGP
Copy link
Contributor

Replace && in constraints with prose "and" to avoid overfull \hbox

How about replacing them with uses_allocator_v<flat_(multi)map, Allocator>?

Copy link
Contributor

@JohelEGP JohelEGP left a comment

Choose a reason for hiding this comment

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

Here's some refactoring for simpler wording.

source/containers.tex Outdated Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
source/containers.tex Show resolved Hide resolved
@jwakely
Copy link
Member

jwakely commented Jul 25, 2022

How about replacing them with uses_allocator_v<flat_(multi)map, Allocator>?

That would be correct, but I don't think it would improve the specification, as you have to follow an indirection to see what the constraint is actually telling you. Maybe that's OK, as the standard is not a tutorial.

source/containers.tex Show resolved Hide resolved
source/containers.tex Outdated Show resolved Hide resolved
jensmaurer and others added 3 commits August 14, 2022 14:14
 - Replaced "may" with "can" in notes.
 - Replace && in constraints with prose "and" to avoid overfull \hbox
The earlier wording of "containers that the user defines" was not
retained for new additions to the Standard, and it is needlessly
informal. The term "program-defined" was only added recently, whereas
the original wording has essentially existed since the beginnig.
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.

[2022-07 LWG Motion 6] P0429R9 A Standard flat_map P0429 A Standard flatmap
6 participants