-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Renamed "undef" -> "uninit" #71418
Renamed "undef" -> "uninit" #71418
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cramertj (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. Due to 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. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
This does not fully resolve the issue. Those three renames were just examples, I did not actually search for all identifiers that need renaming. Someone still needs to do that work. :) It's a great start though, I am fine with doing this rename in steps. |
I think I have implemented what you asked so far. @RalfJung what about
Is there such a thing as "undefined" variable? i.e. if I replace every instance of "undefined variable" to "uninitialized variable", will they all make sense? |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
I think I need some more context for this, could you point to some occurrences of this term that may have to be replaced? |
r? @oli-obk |
eh, wtf. r? @RalfJung |
You also need to run |
☔ The latest upstream changes (presumably #71200) made this pull request unmergeable. Please resolve the merge conflicts. |
Also you have a whole bunch of merge conflicts... could you please rebase, and re- |
I have since rebased to master, waiting for the test to finish. I had to resolve some merge conflicts so I cannot vouch if all i did was truly just renaming stuff - meaning there might be logic change. Namely, I had to remove a bunch of |
it...checked out perfectly on my end. I did have to force push, could that be an issue?
|
Hm, maybe you need to rebase again? |
This comment has been minimized.
This comment has been minimized.
72b3a5b
to
bfb5e28
Compare
☔ The latest upstream changes (presumably #71958) made this pull request unmergeable. Please resolve the merge conflicts. |
And there's another conflict. :/ Unfortunately renames tend to have that issue, as they touch code in many places. I'll review this ASAP when you are ready. Please don't forget about the 2 open conversations, though (in particular #71418 (comment)). |
src/test/mir-opt/const_prop/control-flow-simplification/rustc.hello.ConstProp.diff
Outdated
Show resolved
Hide resolved
Also the conflicts are not resolved yet according to what GitHub says. |
@hbina not sure if you are aware, there are still unresolved conflicts. |
Sorry, it takes quite some time to run the bless |
It's okay, just wanted to make sure you were not waiting for me. :) Remember to always add |
1. InvalidUndefBytes -> InvalidUninitBytes 2. ScalarMaybeUndef -> ScalarMaybeUninit 3. UndefMask -> InitMask Related issue rust-lang#71193
Looking good, thanks for enduring. :) |
📌 Commit b2a8b39 has been approved by |
thank you for the patience! |
☀️ Test successful - checks-actions, checks-azure |
Tested on commit rust-lang/rust@0f9088f. Direct link to PR: <rust-lang/rust#71418> 💔 miri on windows: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung). 💔 miri on linux: test-pass → build-fail (cc @oli-obk @eddyb @RalfJung).
Related issue #71193