Add Iterator::rfind.

[clarfonthey]

I found it weird that Iterator has rpostition but not rfind. This adds that method.

[rust-highfive]

r? @alexcrichton

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

[bluss]

I think this is good. It's in line with the fold_ok ideas in two ways

• Iterators can special case internal iteration
• In particular, Rev<I> can translate find → rfind and rfind → find

rposition is sort of an anomaly. Should we learn from that by putting this method directly in the DoubleEndedIterator trait? I don't see a downside to that (except the inconsistency with the “misplaced” method Iterator::rposition since it could too have been on DEI.)

[bluss]

Implementation-wise I'd write this directly using while let and self.next_back(). A reason why is that it's then easy to see that we are not taking any detours through the Iterator impl for &mut I.

[alexcrichton]

@bluss could you expand on the point about the location of the method? I vaguely recall that this was a thing in the past but can't recall specifics right now

[bluss]

It's just that isn't it more natural to have a method directly on the trait where the method's functionality is enabled, instead of trait Iterator { fn rfind(&mut self) ... where Self: DoubleEndedIterator }?

Provided method on the supertrait (Iterator) works due to Rust's happiness with cyclical things but isn't it.. a complex thing that doesn't gain you anything. (Just put the method in the trait that has the functionality that enables it).

Since it's a method with a provided implementation, giving a specific impl for it should be easy either way. Though I have not explored it in detail, adding specializations for it is complicated (even incompatible?) if this method sits on Iterator with a where clause instead of being directly in DEI.

[clarfonthey]

I agree with @bluss on the fact that DoubleEndedIterator should contain these methods instead of Iterator, also because it'd help make it more clear at a distance which methods you gain from being double-ended. Also, it'd remove the clutter from the Iterator docs, which is quite overwhelming imho.

Unfortunately, that'd be a super-breaking change because implementors of Iterator can already override these functions themselves. It'd be something to add to Rust 2 if anything.

The reason why I added this to Iterator is for consistency.

[bluss]

I'd prefer to add them rfind in the right trait, instead of consistency with prior errors.

[clarfonthey]

@bluss I feel like it'd be off-putting to find this one method in DoubleEndedIterator while rposition and rev are not.

[alexcrichton]

@bluss ah ok, thanks for the explanation! IIRC one motivation for putting everything on Iterator was for better documentation as now that one trait is the one-stop-shop for all libstd combinators. It sounds like though there's not necessarily a technical reason to go on DEI, just stylistic? In that sense it may make sense to continue precedent and leave it on Iterator?

[clarfonthey]

I personally feel like precedent is good but we should also open up a dedicated issue for eventually moving these to the right place.

[bluss]

I really think that once you recognize an error, you do your best to (1) fix it and (2) not repeat it. I'm not fond of the idea of sticking it in the Iterator trait.

Can't move the existing methods, but we can stop repeating the same error.

@alexcrichton

IIRC one motivation for putting everything on Iterator was for better documentation as now that one trait is the one-stop-shop for all libstd combinators.

I don't think we serve users well this way, it's another inconsistent rule about std they will have to learn. (Any other trait that does the same thing?) The method's location should be decided by where its functionality is enabled, by the construction of the feature, not by how we want to present it.

[clarfonthey]

That's fair. For consistency, maybe we could try moving the other two methods and do a crater run to see the impact?

[alexcrichton]

Er well I guess my point is that I don't think we made an error. I find it quite useful to have a one-stop-shop for all iterator API documentation and would be sad to see it split across a number of traits.

@rust-lang/libs do others have an opinion on this here? The specific question being debated is where a function like this should live. @bluss believes APIs should live on the trait they're associated with, where I personally prefer to keep all the iterator docs in one place.

[bluss]

It's a well-intentioned kind of “nice” that leaves a spaghetti trail. (Should BufRead extension methods like .lines() instead be placed in Read, and so on?)

[aturon]

Note that rposition was never in DoubleEndedIterator; it requires ExactSizeIterator as well. Here's how it used to look: https://github.com/rust-lang/rust/blob/0.12.0/src/libcore/iter.rs#L708

Of course, that's not really the right location, and having ExactSizeIterator inherit from DoubleEndedIterator was wrong.

So I think the real question here is: what should be done for a method that's available only when two distinct traits (ExactSizeIterator and DoubleEndedIterator) are implemented? We could always have a third extension trait that inherits from both with a blanket impl, but that seems unfortunate and requires additional imports. But there's no good reason for the method to live on one or the other of the base traits.

Having the method live on Iterator gives it a reasonably canonical location, and means that you get access to it without importing any additional traits. (Granted we could use the prelude to work around the import issue).

[bluss]

rfind doesn't have the multiple choice issue, it's unambiguously DEI (which is in the prelude).

[aturon]

@bluss Understood -- was just trying to give more historical context.

@alexcrichton Given the context above, do you still feel strongly that the method should live on Iterator? I'm personally open to either choice for this method.

[clarfonthey]

IMHO the only benefit to having more functions in iterator is being able to use Iterator::function as an argument to a function. That's actually the major breaking part.

However, in the case of this function, it's worth noting that because it's unstable, it can be moved after this PR is merged. We could always put this sort of discussion in the tracking issue.

[alexcrichton]

Ok let's just put this method on DEI then

[clarfonthey]

It has been moved.

[alexcrichton]

@bors: r+

[bors]

📌 Commit 3cf485c has been approved by alexcrichton

[bluss]

Thanks for working on this. There's a chance I'll propose rfold in a bit..

[bors]

⌛ Testing commit 3cf485c with merge 1b5c7ac...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 1b5c7ac to master...