-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Added Self where possible #29965
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
Added Self where possible #29965
Conversation
In this commit, I added `Self` as the return value for implementations that returned their own type as it seems much more readable IMO. Additionally for the implementations of Iterator and IntoIter, I used Self::Item and Self::IntoIter
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gankro (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Thanks for the PR! This may want to hold off, however, for a stylistic guideline on the usage of |
That's understandable. Does this stand even in implementations of the Just out of curiosity, would it be possible to have |
i'm a big fan of Self, and think all of these uses are uncontroversial. |
Oops! |
If there ever is a consensus on the style for |
@gankro Does it affect on compilation speed for users' code? |
I would be shocked if it showed up in compilation times, as it is trivial to desugar Self by looking at the parent impl block. |
|
All of these changes are to standard trait implementations (so documentation/confusion isn't an issue, it's just DRY) or to private internals of code I'm one of the primary maintainers of (so documentation isn't an issue, and I believe it makes the code more simple). As such I'm accepting these changes. @bors r+ rollup |
📌 Commit 44500c8 has been approved by |
⌛ Testing commit 44500c8 with merge e77df19... |
@bors r- Actually LinkedList::new/append/split_off are being updated here, so those are actual things worth discussing. |
If I may contribute my two cents. Upon further review I think Here's my reasoning: |
☔ The latest upstream changes (presumably #30017) made this pull request unmergeable. Please resolve the merge conflicts. |
Isn't |
@tomaka I use Self basically everywhere I can, and haven't ever had problems. I've only seen a bug in an unused-type lint, but the example was degenerate. |
This PR has merge conflicts, can we get a rebase? |
Hey @crossroads1112, sorry for the delay! We're happy to land this if you revert the two public methods on LinkedList (split_off and append). |
@tomaka I'm not familiar with any bugs? Many of us use it all the time. Perhaps associated item stuff..? (which is buggy) |
Done |
@crossroads1112 hey thanks! It seems there's some merge conflicts. Can you resolve them and squash all the commits? |
Closing PR as it is replaced by this one |
In this commit, I added
Self
as the return value for implementations that returned their own type as it seems much more readable IMO. Additionally for the implementations of Iterator and IntoIter, I used Self::Item and Self::IntoIterEDIT: This is in the LinkedList implementation specifically