Implement specialized nth_back() for Zip#68199
Implement specialized nth_back() for Zip#68199JohnTitor wants to merge 1 commit intorust-lang:masterfrom
nth_back() for Zip#68199Conversation
rust-highfive
added
the
S-waiting-on-review
|
r? @matthewjasper as per recent review of std specializations |
| })) | ||
| .nth_back(3); | ||
| assert_eq!(value, Some((30, 4000))); | ||
| assert_eq!(a, vec![6, 6, 5, 5, 4, 4, 3]); |
There was a problem hiding this comment.
This is not correct and appears to be a bug that exists in the current next_back implementation. This should only contain each number once.
There was a problem hiding this comment.
Hm, so the current nth_back implementation is also wrong?
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=ad07ad6a7077e2ba38eb232eed5e15bc
There was a problem hiding this comment.
It's calling next_back, so yes.
There was a problem hiding this comment.
Filed it as #68536 (I think this PR encounters another roadblock that Niko pointed out, so left a new issue).
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
I'm not entirely sure what we should do about this soundness problem; it seems like prioritizing work on specialization may be necessary to avoid reverting a lot of the existing specializations in the standard library.
| } | ||
| } | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
Unfortunately, this specialization is unsound, though it's not a pre-existing problem, and not specific to this PR. See my comment here for a bit more context.
There was a problem hiding this comment.
Maybe -- there are specific patterns that are sound -- but you have to specialize not based on traits but based on concrete types. e.g., specializing for vec::Iter or something would be fine. But maybe there are so many types here that this is not practical?
There was a problem hiding this comment.
but you have to specialize not based on traits but based on concrete types. e.g., specializing for vec::Iter or something would be fine.
Yeah, it'd be an alternative. But unfortunately, I have no time to find alternative implementation. If I find some time, I'll re-open or submit another PR. Thanks for the review, Niko!
6 participants
Rework of #60574 and part of #54054 (it can be closed by this?)
r? @scottmcm