-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
refactor: removing custom nth Zip fn #79173
Conversation
r? @kennytm (rust_highfive has picked a reviewer for you, use r? to override) |
Not sure if someone can just kill the CI? It has been going for 1h 30mins and it usually consistency for others takes 40mins. I think my misunderstanding of traits/impl has caused an infinite loop maybe of nth() calling itself? |
Maybe let's figure out if this cleanup is really possible first (having the CI pass). |
@kennytm I have read the documentation and various posts but can not seem to find how to implement my desired behaviour above, would you know how? As I was looking at writing a specialised Just concerned about the implementations changing in |
☔ The latest upstream changes (presumably #79319) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
@DeveloperC286 Ping from triage! What's the current status of this? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Could not figure out how to clean it up as much as I wanted. However I think the PR I have now is an improvement so this is good for review now 👍 |
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
☔ The latest upstream changes (presumably #81089) made this pull request unmergeable. Please resolve the merge conflicts. |
Hi @kennytm any chance you can take a look? I think my PR makes the code more simple/clean. Let me know your thoughts and I can just close the PR if you think it does not 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me with nit
@bors r+ rollup=iffy |
📌 Commit 152f500 has been approved by |
…nnytm refactor: removing custom nth Zip fn Noticed `super_nth()` that seems very similar to `nth()` in `iterator.rs`. If you look at `nth()` in `iterator.rs` before the commit `ecacc7534b6bf50205c37c89402565b82d95a257` `super_nth()` looks exactly the same as `fn nth()` in `iterator.rs`. I may be misunderstanding something, but I think `super_nth()` can just be removed.
@bors r- failed in #81559 (comment) I believe |
@DeveloperC286 Ping from triage: Seems this pr is breaking other crates from compilation in the rollup above. Would you mind investigating into this? Thanks! |
This comment has been minimized.
This comment has been minimized.
☔ The latest upstream changes (presumably #83580) made this pull request unmergeable. Please resolve the merge conflicts. |
ping from triage: |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ping from triage: |
Sorry struggled for a long time to reproduce the issue by bors in the roll up, but now the issue is present in the regular test suite. Using the |
This comment has been minimized.
This comment has been minimized.
When the iterator nth() method was updated it was not in zip. Zip needs to implement nth() for the trusted length specialised implementation.
The job Click to see the possible cause of the failure (guessed by this bot)
|
ping from triage: |
@DeveloperC286 I'm gonna close this due to inactivity. Feel free to reopen or create a new pr when you got time to work on this, thanks! |
Noticed
super_nth()
that seems very similar tonth()
initerator.rs
.If you look at
nth()
initerator.rs
before the commitecacc7534b6bf50205c37c89402565b82d95a257
super_nth()
looks exactly the same asfn nth()
initerator.rs
.I may be misunderstanding something, but I think
super_nth()
can just be removed.