Skip to content

Conversation

acrrd
Copy link
Contributor

@acrrd acrrd commented May 2, 2019

Implementation of nth_back for Chain.
Part of #54054

@rust-highfive
Copy link
Contributor

r? @withoutboats

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 2, 2019
@acrrd
Copy link
Contributor Author

acrrd commented May 2, 2019

r? @scottmcm

@koalatux
Copy link
Contributor

koalatux 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
Copy link
Contributor Author

acrrd commented May 7, 2019

What is the best way to test both implementations?

@Dylan-DPC-zz
Copy link

ping from triage @scottmcm waiting for your review on this

@timvermeulen
Copy link
Contributor

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 issues/54054_chain branch from 04db884 to d04343f Compare May 28, 2019 22:15
@acrrd
Copy link
Contributor Author

acrrd commented May 28, 2019

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

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Jun 18, 2019

Ping from triage, still waiting on your review @scottmcm

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

@scottmcm
Copy link
Member

scottmcm 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
Copy link
Contributor

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

@acrrd acrrd force-pushed the issues/54054_chain branch 2 times, most recently from b625514 to 558f909 Compare July 2, 2019 10:35
@acrrd
Copy link
Contributor Author

acrrd commented Jul 8, 2019

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

@totsteps
Copy link

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

@fmckeogh
Copy link
Contributor

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

@SimonSapin

@Dylan-DPC-zz
Copy link

r? @SimonSapin

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

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
Copy link
Member

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
Copy link
Member

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

@acrrd acrrd force-pushed the issues/54054_chain branch from 558f909 to 7b6ad60 Compare August 11, 2019 17:16
@acrrd
Copy link
Contributor Author

acrrd commented Aug 11, 2019

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

@SimonSapin
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Aug 16, 2019

📌 Commit 7b6ad60 has been approved by SimonSapin

@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 16, 2019
Centril added a commit to Centril/rust that referenced this pull request Aug 16, 2019
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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.