-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Implement push_mut
#135975
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
base: master
Are you sure you want to change the base?
Implement push_mut
#135975
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ibraheemdev (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Sorry if I did this wrong, it's my first time! |
This comment has been minimized.
This comment has been minimized.
A single trailing whitespace. Oops. |
This comment has been minimized.
This comment has been minimized.
I'm not sure we should be touching |
Alright, I'll do that soon. |
I finally got around to duplicating the implementation. College has been eating my time like a child with Halloween candy, sorry. |
@rustbot review |
library/alloc/src/vec/mod.rs
Outdated
/// | ||
/// let mut vec = vec![]; | ||
/// // Due to current borrow checker limitations (see -Zpolonius), this is the only way to spell this right now. | ||
/// let last = if let Some(v) = vec.last_mut() { v } else { vec.push_mut(0) }; |
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.
I'm not sure I understand the point of this.
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.
There should probably be a better example, yeah. Will update soon!
@balt-dev |
I'm sorry, college has been quite a time sink for me and I haven't had the time to follow up as of recent - I will when I can, promise! |
It's been a rough few months. ._. |
This comment has been minimized.
This comment has been minimized.
hm. |
I think I messed up my fork so bad I'll have to reset it and put my code in from scratch in order to get rid of the merge commit. At least it's copy/pasteable? |
This comment has been minimized.
This comment has been minimized.
...again? |
Sigh.... |
@traviscross How'd you fix the submodules? |
This comment has been minimized.
This comment has been minimized.
The easiest way is something like: # Make sure you have rust-lang's `master` available
git remote add upstream https://github.com/rust-lang/rust.git
git fetch upstream master
# Do a soft reset to the point where you forked off from master.
# Mild warning: this will keep the diff but get rid of the new
# commits (the files will show up as having unstaged changes)
git reset "$(git merge-base HEAD upstream/master)"
# Just reset submodules
git submodule deinit --all --force
# Add the changes you actually want and create a new commit
git add library
git commit
# Update this PR, which will replace the three commits here with
# the one new commit
git push --force-with-lease
For future reference, it's usually better practice to always work on branches rather than your fork's |
☔ The latest upstream changes (presumably #142109) made this pull request unmergeable. Please resolve the merge conflicts. |
That probably fixed the merge conflict. |
@tgross35 Thank you so much! |
Co-authored-by: Ibraheem Ahmed <[email protected]>
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.
Hm. |
---- library/alloc/src/vec/mod.rs - vec::Vec<T,A>::push_mut (line 2572) stdout ----
error[E0614]: type `()` cannot be dereferenced
--> library/alloc/src/vec/mod.rs:2579:1
|
10 | *last = 4;
| ^^^^^ can't be dereferenced
error: aborting due to 1 previous error
For more information about this error, try `rustc --explain E0614`. Ah. @ibraheemdev The test you provided in the review was broken - it used |
This comment has been minimized.
This comment has been minimized.
@ibraheemdev ...did you even test that suggested test before suggesting it? |
Please keep in mind that reviews are thoughtful and often suggest a general shape, but it's really up to the PR author to make sure they work and tweak them as needed. Reviewers won't be checking out and building your branch. It's reasonably easy to run tests locally, |
Yeah, @ibraheemdev sorry for lashing out. |
Implementation of #135974.