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

Add `Iterator::take_while_map` #66577

Open
wants to merge 4 commits into
base: master
from

Conversation

@WaffleLapkin
Copy link

WaffleLapkin commented Nov 20, 2019

In Iterator trait there is *_map version of filterfilter_map, however, there is no *_map version of take_while, that can also be useful.

Use cases

In my code, I've found that I need to iterate through iterator of Options, stopping on the first None. So I've written code like this:

let arr = [Some(4), Some(10), None, Some(3)];
let mut iter = arr.iter()
    .take_while(|x| x.is_some())
    .map(|x| x.unwrap());

assert_eq!(iter.next(), Some(4));
assert_eq!(iter.next(), Some(10));
assert_eq!(iter.next(), None);
assert_eq!(iter.next(), None);

Thit code

  1. isn't clean
  2. In theory, can generate bad bytecode (I'm actually not sure, but I think that unwrap would generate additional branches with panic!)

The same code, but with take_while_map:

let arr = [Some(4), Some(10), None, Some(3)];
let mut iter = arr.iter().take_while_map(std::convert::identity);

Also, take_while_map can be useful when converting something (as in examples).

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 20, 2019

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

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 20, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-11-20T16:02:59.1984724Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-20T16:03:00.0431378Z ##[command]git config gc.auto 0
2019-11-20T16:03:00.0435518Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-20T16:03:00.0440007Z ##[command]git config --get-all http.proxy
2019-11-20T16:03:00.0442520Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66577/merge:refs/remotes/pull/66577/merge
---
2019-11-20T16:08:40.8537373Z    Compiling serde_json v1.0.40
2019-11-20T16:08:42.6561840Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-11-20T16:08:52.4864382Z     Finished release [optimized] target(s) in 1m 20s
2019-11-20T16:08:52.4959577Z tidy check
2019-11-20T16:08:53.3217507Z tidy error: /checkout/src/libcore/iter/traits/iterator.rs:7: line longer than 100 chars
2019-11-20T16:08:53.3236806Z tidy error: /checkout/src/libcore/iter/traits/iterator.rs: too many lines (3091) (add `// ignore-tidy-filelength` to the file to suppress this error)
2019-11-20T16:08:54.9859518Z some tidy checks failed
2019-11-20T16:08:54.9860731Z Found 441 error codes
2019-11-20T16:08:54.9861215Z Found 0 error codes with no tests
2019-11-20T16:08:54.9861484Z Done!
2019-11-20T16:08:54.9861484Z Done!
2019-11-20T16:08:54.9861670Z 
2019-11-20T16:08:54.9861852Z 
2019-11-20T16:08:54.9862861Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-20T16:08:54.9863399Z 
2019-11-20T16:08:54.9863599Z 
2019-11-20T16:08:54.9865764Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-20T16:08:54.9866051Z Build completed unsuccessfully in 0:01:23
2019-11-20T16:08:54.9866051Z Build completed unsuccessfully in 0:01:23
2019-11-20T16:08:54.9916763Z == clock drift check ==
2019-11-20T16:08:54.9926882Z   local time: Wed Nov 20 16:08:54 UTC 2019
2019-11-20T16:08:56.5153482Z   network time: Wed, 20 Nov 2019 16:08:56 GMT
2019-11-20T16:08:56.5153590Z == end clock drift check ==
2019-11-20T16:08:57.8028650Z 
2019-11-20T16:08:57.8125032Z ##[error]Bash exited with code '1'.
2019-11-20T16:08:57.8154642Z ##[section]Starting: Checkout
2019-11-20T16:08:57.8156372Z ==============================================================================
2019-11-20T16:08:57.8156422Z Task         : Get sources
2019-11-20T16:08:57.8156485Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Nov 20, 2019

That method name is a lot, but it does seem potentially useful?

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Nov 20, 2019

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rkruppe

This comment has been minimized.

Copy link
Member

rkruppe commented Nov 20, 2019

For the record, this was previously proposed in #65864. It was not merged because I (randomly assigned reviewer) and the two T-libs members who voiced an opinion after I pinged the team were lukewarm on adding it to std instead of itertools. Take from that what you will.

fmt
@WaffleLapkin

This comment has been minimized.

Copy link
Author

WaffleLapkin commented Nov 20, 2019

For the record, this was previously proposed in #65864.

Oh. When I first started thinking of take_while_map the #65864 didn't exist yet, so when I finally started implementing this, I didn't search for similar PRs 😅

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 20, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-11-20T16:46:43.5713784Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-20T16:46:43.5902895Z ##[command]git config gc.auto 0
2019-11-20T16:46:43.5981240Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-20T16:46:43.6036648Z ##[command]git config --get-all http.proxy
2019-11-20T16:46:43.6185233Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66577/merge:refs/remotes/pull/66577/merge
---
2019-11-20T17:44:31.7630761Z .................................................................................................... 1500/9257
2019-11-20T17:44:37.5221004Z .................................................................................................... 1600/9257
2019-11-20T17:44:45.9818912Z .................................................................................................... 1700/9257
2019-11-20T17:44:54.8031210Z ........i........................................................................................... 1800/9257
2019-11-20T17:45:01.1548601Z .............................................................................................iiiii.. 1900/9257
2019-11-20T17:45:22.9275058Z .................................................................................................... 2100/9257
2019-11-20T17:45:25.3964813Z .................................................................................................... 2200/9257
2019-11-20T17:45:28.0215234Z .................................................................................................... 2300/9257
2019-11-20T17:45:34.2310297Z .................................................................................................... 2400/9257
---
2019-11-20T17:48:23.1748153Z ..............................................................................................i..... 4700/9257
2019-11-20T17:48:29.3221867Z ..........i......................................................................................... 4800/9257
2019-11-20T17:48:38.6455528Z .................................................................................................... 4900/9257
2019-11-20T17:48:43.6645650Z .................................................................................................... 5000/9257
2019-11-20T17:48:53.5826069Z ...................................................................................................i 5100/9257
2019-11-20T17:48:58.2967687Z i.ii...........i.................................................................................... 5200/9257
2019-11-20T17:49:07.8302726Z .................................................................................................... 5400/9257
2019-11-20T17:49:17.8872686Z .................................................................................i.................. 5500/9257
2019-11-20T17:49:25.6117283Z .................................................................................................... 5600/9257
2019-11-20T17:49:31.6284093Z .................................................................................................... 5700/9257
2019-11-20T17:49:31.6284093Z .................................................................................................... 5700/9257
2019-11-20T17:49:41.6692653Z ...................................................................ii...i..ii...........i........... 5800/9257
2019-11-20T17:50:03.3575499Z .................................................................................................... 6000/9257
2019-11-20T17:50:11.8691906Z .................................................................................................... 6100/9257
2019-11-20T17:50:11.8691906Z .................................................................................................... 6100/9257
2019-11-20T17:50:16.5102664Z ........................................................................................i..ii....... 6200/9257
2019-11-20T17:50:40.8725552Z .................................................................................................... 6400/9257
2019-11-20T17:50:48.9960026Z ........................................................i........................................... 6500/9257
2019-11-20T17:50:51.0637128Z .................................................................................................... 6600/9257
2019-11-20T17:50:53.2586028Z .............................................i...................................................... 6700/9257
---
2019-11-20T17:56:00.9160271Z  finished in 5.589
2019-11-20T17:56:00.9334182Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-11-20T17:56:01.1197149Z 
2019-11-20T17:56:01.1197450Z running 156 tests
2019-11-20T17:56:04.0229031Z iiii....iii......iii..iiii...i.............................i..i..................i....i...........ii 100/156
2019-11-20T17:56:05.8936726Z .i.i..iiii..............i.........iii.i.........ii......
2019-11-20T17:56:05.8937275Z 
2019-11-20T17:56:05.8937416Z  finished in 4.960
2019-11-20T17:56:05.9122436Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-11-20T17:56:06.0793374Z 
---
2019-11-20T17:56:08.0237660Z  finished in 2.111
2019-11-20T17:56:08.0446945Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-11-20T17:56:08.2266834Z 
2019-11-20T17:56:08.2266973Z running 9 tests
2019-11-20T17:56:08.2267764Z iiiiiiiii
2019-11-20T17:56:08.2268363Z 
2019-11-20T17:56:08.2268411Z  finished in 0.183
2019-11-20T17:56:08.2458278Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-11-20T17:56:08.4272746Z 
---
2019-11-20T17:56:27.6555083Z  finished in 19.410
2019-11-20T17:56:27.6772305Z Check compiletest suite=debuginfo mode=debuginfo-gdb+lldb (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-11-20T17:56:27.8712793Z 
2019-11-20T17:56:27.8713052Z running 123 tests
2019-11-20T17:56:53.2097562Z .iiiii...i.....i..i...i..i.i.i..i.ii..i.i.....i..i....ii..........iiii..........i...ii...i.......ii. 100/123
2019-11-20T17:56:58.0850937Z i.i.i......iii.i.....ii
2019-11-20T17:56:58.0851426Z 
2019-11-20T17:56:58.0858207Z  finished in 30.408
2019-11-20T17:56:58.0867714Z Uplifting stage1 rustc (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-11-20T17:56:58.0869067Z Copying stage2 rustc from stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
2019-11-20T18:07:43.5168579Z 
2019-11-20T18:07:43.5246128Z  finished in 123.111
2019-11-20T18:07:43.5266358Z Testing core stage1 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-11-20T18:07:43.7181960Z    Compiling core v0.0.0 (/checkout/src/libcore)
2019-11-20T18:07:49.3133390Z error[E0658]: use of unstable library feature 'iter_take_while_map': recently added
2019-11-20T18:07:49.3134437Z     --> src/libcore/../libcore/tests/iter.rs:1482:26
2019-11-20T18:07:49.3134860Z      |
2019-11-20T18:07:49.3135346Z 1482 |     assert_eq!(c.clone().take_while_map(|_| None).size_hint(), (0, None));
2019-11-20T18:07:49.3136191Z      |
2019-11-20T18:07:49.3136636Z      = help: add `#![feature(iter_take_while_map)]` to the crate attributes to enable
2019-11-20T18:07:49.3136837Z 
2019-11-20T18:07:49.3136837Z 
2019-11-20T18:07:49.3340493Z error[E0658]: use of unstable library feature 'iter_take_while_map': recently added
2019-11-20T18:07:49.3341399Z     --> src/libcore/../libcore/tests/iter.rs:1497:27
2019-11-20T18:07:49.3342222Z      |
2019-11-20T18:07:49.3342696Z 1497 |     assert_eq!(vi.clone().take_while_map(|_| None).size_hint(), (0, Some(10)));
2019-11-20T18:07:49.3343518Z      |
2019-11-20T18:07:49.3343958Z      = help: add `#![feature(iter_take_while_map)]` to the crate attributes to enable
2019-11-20T18:07:49.3344153Z 
2019-11-20T18:07:54.9193592Z error: aborting due to 2 previous errors
---
2019-11-20T18:07:55.0457970Z   local time: Wed Nov 20 18:07:55 UTC 2019
2019-11-20T18:07:55.3401698Z   network time: Wed, 20 Nov 2019 18:07:55 GMT
2019-11-20T18:07:55.3402586Z == end clock drift check ==
2019-11-20T18:07:55.9264418Z 
2019-11-20T18:07:55.9367554Z ##[error]Bash exited with code '1'.
2019-11-20T18:07:55.9403009Z ##[section]Starting: Checkout
2019-11-20T18:07:55.9404901Z ==============================================================================
2019-11-20T18:07:55.9404946Z Task         : Get sources
2019-11-20T18:07:55.9404984Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Nov 20, 2019

If we want to add this I think map_while is both clearer and briefer than take_while_map.

#[derive(Clone)]
pub struct TakeWhileMap<I, P> {
iter: I,
flag: bool,

This comment has been minimized.

Copy link
@AnthonyMikh

AnthonyMikh Nov 22, 2019

Contributor

Maybe not flag but finished?

This comment has been minimized.

Copy link
@WaffleLapkin

WaffleLapkin Nov 22, 2019

Author

This field was copy-pasted from TakeWhile 🤷‍♀️ But I agree that finished is more clear than flag (actually when I was first reading TakeWhile sources it took me a while to understand the meaning of the flag)

}
}

#[unstable(feature = "iter_take_while_map", reason = "recently added", issue = "0")]

This comment has been minimized.

Copy link
@AnthonyMikh

AnthonyMikh Nov 22, 2019

Contributor

Stability attributes currently don't work with trait impls

This comment has been minimized.

Copy link
@WaffleLapkin

WaffleLapkin Nov 22, 2019

Author

Why then there is a #[stable(feature = "rust1", since = "1.0.0")] on Iterator impl for TakeWhile?

This comment has been minimized.

Copy link
@dtolnay

dtolnay Nov 28, 2019

Member

It's unstability attributes that don't work. ;) There is no such thing as an unstable trait impl.

if self.flag {
None
} else {
let x = self.iter.next()?;
match (self.predicate)(x) {
res @ Some(_) => res,
None => {
self.flag = true;
None
}
}
}
Comment on lines +1652 to +1663

This comment has been minimized.

Copy link
@AnthonyMikh

AnthonyMikh Nov 22, 2019

Contributor

Can be simpler:

if self.flag {
    return None
}

let x = self.iter.next()?;
let ret = (self.predicate)(x);
self.flag = ret.is_none();
ret

This comment has been minimized.

Copy link
@WaffleLapkin

WaffleLapkin Nov 22, 2019

Author

I think it would be even better to use if+else instead of if+return:

if self.flag {
    None
} else {
    let x = self.iter.next()?;
    let ret = (self.predicate)(x);
    self.flag = ret.is_none();
    ret
}
@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Dec 6, 2019

Ping from triage:
@sfackler - Can you please review this PR?
Thanks.

@sfackler

This comment has been minimized.

Copy link
Member

sfackler commented Dec 8, 2019

@JohnCSimon this is still pending FCP waiting on @Kimundi, @KodrAus, @SimonSapin, @dtolnay, and @withoutboats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.