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

Add Iterator::map_windows #94667

Merged
merged 1 commit into from
Aug 13, 2023
Merged

Add Iterator::map_windows #94667

merged 1 commit into from
Aug 13, 2023

Conversation

frank-king
Copy link
Contributor

@frank-king frank-king commented Mar 6, 2022 •
edited
Loading

Tracking issue: #87155.

This is inherited from the old PR #82413.

Unlike #82413, this PR implements the MapWindows to be lazy: only when pulling from the outer iterator, .next() of the inner iterator will be called.

Implementaion Steps

  • Implement MapWindows to keep the iterators' Laziness contract.
  • Fix the known bug of memory access error.
  • Full specialization of iterator-related traits for MapWindows.
    • Iterator::size_hint,
    • Iterator::count,
    • ExactSizeIterator (when I: ExactSizeIterator),
    • TrustedLen (when I: TrustedLen),
    • FusedIterator,
    • Iterator::advance_by,
    • Iterator::nth,
    • ...
  • More tests and docs.

Unresolved Questions:

  • Is there any more iterator-related traits should be specialized?
  • Is the double-space buffer worth?
  • Should there be rmap_windows or something else?
  • Taking GAT for consideration, should the mapper function be FnMut(&[I::Item; N]) -> R or something like FnMut(ArrayView<'_, I::Item, N>) -> R? Where ArrayView is mentioned in Extending core::ops::Index to return non-references generic-associated-types-initiative#2.
    • It can save memory, only the same size as the array window is needed,
    • It is more efficient, which requires less data copies,
    • It is possibly compatible with the GATified version of LendingIterator::windows.
    • But it prevents the array pattern matching like iter.map_windows(|_arr: [_; N]| ()), unless we extend the array pattern to allow matching the ArrayView.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 6, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@frank-king frank-king force-pushed the feature/iter_map_windows branch 2 times, most recently from 455690c to 32bab75 Compare March 7, 2022 10:16
@frank-king frank-king marked this pull request as ready for review March 7, 2022 15:16
@frank-king
Copy link
Contributor Author

r? @rust-lang/libs
r? @LukasKalbertodt
r? @dtolnay

@rust-highfive rust-highfive assigned yaahc and unassigned m-ou-se Mar 7, 2022
@yaahc yaahc added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Mar 18, 2022
@bors
Copy link
Contributor

bors commented Apr 8, 2022

☔ The latest upstream changes (presumably #95798) made this pull request unmergeable. Please resolve the merge conflicts.

@frank-king frank-king marked this pull request as draft April 14, 2022 09:21
@bors
Copy link
Contributor

bors commented Jun 10, 2022

☔ The latest upstream changes (presumably #91970) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 24, 2022
@Mark-Simulacrum
Copy link
Member

Going to mark as waiting-on-author since it's in draft and has some conflicts -- not really clear what the state of this PR is though. If you'd like a review, please leave a comment with @rustbot ready.

@frank-king
Copy link
Contributor Author

Got busy recently.

Actually, I think there should be more tests, while I'm not sure what are the best ways to test it. Maybe a review could help.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 24, 2022
@dtolnay
Copy link
Member

dtolnay commented Dec 23, 2022

Reassigning yaahc's reviews as she has stepped down from the review rotation.

r? rust-lang/libs-api

@rustbot rustbot assigned BurntSushi and unassigned yaahc Dec 23, 2022
@dtolnay
Copy link
Member

dtolnay commented Jan 22, 2023

Reassigning BurntSushi's reviews because based on git log -i --grep=r=burntsushi the last time they did rust-lang/rust reviews was over 3 years ago.

r? rust-lang/libs-api

@GuillaumeGomez
Copy link
Member

It failed in #111760. wasm doesn't support unwinding panics.

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 19, 2023
@Emilgardis
Copy link
Contributor

The fix for that is adding #[cfg(not(panic = "abort"))] to the tests that need unwinding

@frank-king
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2023
@Mark-Simulacrum
Copy link
Member

r=me with a rebase

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2023
@Easyoakland Easyoakland mentioned this pull request Aug 7, 2023
Closed
@frank-king
Copy link
Contributor Author

frank-king commented Aug 10, 2023 •
edited
Loading

Didn't see that message, sorry for the long wait.

@rustbot ready.

r? @Mark-Simulacrum

@rustbot
Copy link
Collaborator

rustbot commented Aug 10, 2023

Could not assign reviewer from: Mark-Simulacrum.
User(s) Mark-Simulacrum are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 10, 2023
@frank-king @LukasKalbertodt
Add Iterator::map_windows
97c953f 
This is inherited from the old PR.

This method returns an iterator over mapped windows of the starting
iterator. Adding the more straight-forward `Iterator::windows` is not
easily possible right now as the items are stored in the iterator type,
meaning the `next` call would return references to `self`. This is not
allowed by the current `Iterator` trait design. This might change once
GATs have landed.

The idea has been brought up by @m-ou-se here:
https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Iterator.3A.3A.7Bpairwise.2C.20windows.7D/near/224587771

Co-authored-by: Lukas Kalbertodt <lukas.kalbertodt@gmail.com>
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 13, 2023

📌 Commit 97c953f has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2023
@GuillaumeGomez GuillaumeGomez mentioned this pull request Aug 13, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 13, 2023
@bors
Auto merge of rust-lang#114786 - GuillaumeGomez:rollup-0cos5gn, r=Gui…
1b198b3 
…llaumeGomez

Rollup of 5 pull requests

Successful merges:

 - rust-lang#94667 (Add `Iterator::map_windows`)
 - rust-lang#114069 (Allow using external builds of the compiler-rt profile lib)
 - rust-lang#114354 (coverage: Store BCB counter info externally, not directly in the BCB graph)
 - rust-lang#114625 (CI: use smaller machines in PR runs)
 - rust-lang#114777 (Migrate GUI colors test to original CSS color format)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7f787e3 into rust-lang:master Aug 13, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 13, 2023
@bors
Copy link
Contributor

bors commented Aug 13, 2023

⌛ Testing commit 97c953f with merge 1b198b3...

@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Emilgardis Emilgardis Emilgardis left review comments

@dtolnay dtolnay dtolnay left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@dtolnay dtolnay

@Mark-Simulacrum Mark-Simulacrum

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.73.0
Development

Successfully merging this pull request may close these issues.