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 custom nth_back for Chain #60492

Merged
merged 1 commit into from Aug 16, 2019

Conversation

@acrrd
Copy link
Contributor

commented May 2, 2019

Implementation of nth_back for Chain.
Part of #54054

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 2, 2019

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

@koalatux

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Nice! Is it possible with the current state of default impl to also have a more specific implementation, where B also implements ExactSizeIterator?

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

What is the best way to test both implementations?

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented May 20, 2019

ping from triage @scottmcm waiting for your review on this

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

You're not mutating self.b in any way when n >= self.b.len() in the specialized version, so if the state is ChainState::Back, I don't think calling nth_back with a too large value will actually empty the iterator (even though it will return None). I haven't tested it, but I think this will fail:

let mut iter = std::iter::empty().chain(vec![0, 0]);
iter.next();        // to change the state to ChainState::Back
iter.nth_back(100); // to empty the iterator
assert_eq!(iter.next(), None);

I think you'll need to call self.b.nth_back(n) regardless of the length of b.

@acrrd acrrd force-pushed the acrrd:issues/54054_chain branch from 04db884 to d04343f May 28, 2019

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

@timvermeulen I added tests for that and fixed the specialization. Thanks!

@jonas-schievink

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Ping from triage, still waiting on your review @scottmcm

(or someone else from @rust-lang/libs)

@scottmcm

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

This is introducing a default fn on an Iterator impl that's not there in the analogous forward implementation of the method, so I'd like someone from libs to take this. *rolls dice*

r? @Kimundi

@rust-highfive rust-highfive assigned Kimundi and unassigned scottmcm Jul 2, 2019

@timvermeulen

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Presumably there could also be a specialized nth when A: ExactSizeIterator?

@acrrd acrrd force-pushed the acrrd:issues/54054_chain branch 2 times, most recently from b625514 to 558f909 Jul 2, 2019

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

What could be the problem in not having nth also be specialized for ExactSizeIterator?

@gagan0723

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

ping from triage, @Kimundi @Centril any updates on this?

@chocol4te

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

Second ping from wg-triage, pinging random member of T-libs per procedure.

@SimonSapin

@Dylan-DPC

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

@rust-highfive rust-highfive assigned SimonSapin and unassigned Kimundi Aug 1, 2019

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Per discussion in #62483 (comment) it’s not clear that we want to use specialization in public trait (as opposed to a private trait that only optimize an implementation detail) while specialization itself is still unstable, or at least while it has significant design issue still open.

CC @alexcrichton

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

I'm personally not the biggest fan of so aggressively using specialization myself, but at a bare minimum the general convention we've had (or so I think) is that all specialization is done internally in the standard library and no public-facing function is listed as default

@JohnCSimon

This comment has been minimized.

Copy link

commented Aug 10, 2019

Ping from triage @acrrd @SimonSapin
Hi! This has sat idle for awhile. Is this close to being resolved?

@acrrd acrrd force-pushed the acrrd:issues/54054_chain branch from 558f909 to 7b6ad60 Aug 11, 2019

@acrrd

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

After the comment of @SimonSapin and @alexcrichton I removed the specialization

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

📌 Commit 7b6ad60 has been approved by SimonSapin

Centril added a commit to Centril/rust that referenced this pull request Aug 16, 2019

Rollup merge of rust-lang#60492 - acrrd:issues/54054_chain, r=SimonSapin
Add custom nth_back for Chain

Implementation of nth_back for Chain.
Part of rust-lang#54054

bors added a commit that referenced this pull request Aug 16, 2019

Auto merge of #63640 - Centril:rollup-yeb8o66, r=Centril
Rollup of 10 pull requests

Successful merges:

 - #60492 (Add custom nth_back for Chain)
 - #61780 (Finalize the error type for `try_reserve`)
 - #63495 ( Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`.)
 - #63525 (Make sure that all file loading happens via SourceMap)
 - #63595 (add sparc64-unknown-openbsd target)
 - #63604 (Some update for vxWorks)
 - #63613 (Hygienize use of built-in macros in the standard library)
 - #63632 (A couple of comment fixes.)
 - #63634 (ci: properly set the job name in CPU stats)
 - #63636 (ci: move linkcheck from mingw-2 to mingw-1)

Failed merges:

r? @ghost

@bors bors merged commit 7b6ad60 into rust-lang:master Aug 16, 2019

5 checks passed

Travis CI - Pull Request Build Passed
Details
pr Build #20190811.21 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.