-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Miri: rename "undef" -> "uninit" #71193
Comments
@rustbot claim |
This will be quite a large change, especially if tests needs to be updated too. And what about the stuff in |
I don't think any tests need to be updated, as there should be no user-visible change. And we're not renaming LLVM, just our own Miri types and fields! If you are grepping for this, only grep in |
1. InvalidUndefBytes -> InvalidUninitBytes 2. ScalarMaybeUndef -> ScalarMaybeUninit 3. UndefMask -> UninitMask Resolves rust-lang#71193
This test will fail
|
@hbina thanks a lot for working on this! |
Another type we will want to rename in a second batch is |
@hbina just a small note: before working on an issue, please check if someone else has claimed it (if it is assigned to rustbot) :) |
1. InvalidUndefBytes -> InvalidUninitBytes 2. ScalarMaybeUndef -> ScalarMaybeUninit 3. UndefMask -> InitMask Related issue rust-lang#71193
Renamed "undef" -> "uninit" 1. InvalidUndefBytes -> InvalidUninitBytes 2. ScalarMaybeUndef -> ScalarMaybeUninit 3. UndefMask -> InitMask Related issue rust-lang#71193
All right, the first round of renames landed. :) Thanks @hbina! For the second round, here are some more names in
|
yea,
this makes me wonder whether it should be
|
I guess it depends on the caller which naming is more apt. But I feel like an "all" statement is intuitively clearer than an "any" statement. |
Hello, I saw that this issue was still open but the last post was 2 months ago, is there a chance I could try and help out? |
@pnadon sure.. 2 months is a short timeframe in rust's perspective :D |
@Dylan-DPC Ok sounds good, From what I understand from the comments above, the two directories I should look in are
And the goal would be to rename parts of names with "undef" in them to "uninit", along with some changes with "def" to "init". Is this correct? |
@pnadon thanks for chiming in! Yes, that sounds right. More specifically, as far as I know, the list at #71193 (comment) is still up-to-date. Post in this thread or open a topic on Rust's Zulip if you need any help; my availability is spotty until mid next week but there'll always be someone around. :) |
no_bytes_init? I assumed it would be synonymous with "all_bytes_uninit" |
I have made commits for the above changes, however I would like to wait until everyone agrees on the new names for below:
I think InitMaskCompressed would be more of a mouthful but also more accurate based on the definition here, so I agree with the latter.
As per my previous comment, no_bytes_init?
I agree. |
I like |
I have also found:
There are also many comments which should be changed, many of them refer to "definedness", which I am not sure what to replace with. |
Yes, those all should become "uninit" I think. For consistency maybe |
…lfJung Miri rename undef uninit Renamed parts of code within the `librustc_middle/mir/interpret/` directory. Related issue [rust-lang#71193](rust-lang#71193)
Miri rename undef uninit The changes made here are related to [issue #71193 on Rust](rust-lang/rust#71193), and the pull request [74664 on Rust](rust-lang/rust#74664). 1. renamed `ScalarMaybeUninit::not_undef` to `check_init` 2. renamed `Immediate::to_scalar_or_undef` to `Immediate::to_scalar_or_uninit`
Sounds good. Should I go ahead and implement those changes? I will also look around for any comments etc. that we may have missed and make sure that we've got them all. |
@pnadon yes please do. :) |
I've done the initial renames, however outside of the two directories (
The former seems to be a fairly straightforward change, I just want to confirm that it's ok. As for the latter, since it is a submodule, I believe I will have to create a pull request for clippy with those changes. Once these issues are resolved, I will look into making the approapriate changes within the Miri repo. |
clippy is not a submodule anymore. You can do these renames either here or in clippy as you prefer |
Sounds good! |
Upon further inspection it does not seem as though there are any references to the above functions within the Miri repository. |
That's great, it makes the rename easier. :)
"initializedness" is awkward, so they should probably be reworded. But from what I can see there is just one occurrence of "definedness" left in the Miri folders:
I'd make this "an entire range of initialization bits" or so. |
Thank you for catching that, I'll look into it! |
Miri builds successfully, I have double-checked my search through both directories and all references to "undef" refer to "undefined behavior" which (unless I'm wrong) is unrelated. I have also changed the last remaining "definedness" to the recommended change. If this all looks good I will open a PR with my changes. |
It sounds great, please open a PR. :) |
Miri: Renamed "undef" to "uninit" Renamed remaining references to "undef" to "uninit" when referring to Miri. Impacted directories are: - `src/librustc_codegen_llvm/consts.rs` - `src/librustc_middle/mir/interpret/` - `src/librustc_middle/ty/print/pretty.rs` - `src/librustc_mir/` - `src/tools/clippy/clippy_lints/src/consts.rs` Upon building Miri based on the new changes it was verified that no changes needed to be made with the Miri project. Related issue rust-lang#71193
Renamed remaining references to "undef" to "uninit" when referring to Miri. Impacted directories are: - src/librustc_codegen_llvm/consts.rs - src/librustc_middle/mir/interpret/ - src/librustc_middle/ty/print/pretty.rs - src/librustc_mir/ - src/tools/clippy/clippy_lints/src/consts.rs Upon building Miri based on the new changes it was verified that no changes needed to be made with the Miri project. Related issue rust-lang#71193
Miri: Renamed "undef" to "uninit" Renamed remaining references to "undef" to "uninit" when referring to Miri. Impacted directories are: - `src/librustc_codegen_llvm/consts.rs` - `src/librustc_middle/mir/interpret/` - `src/librustc_middle/ty/print/pretty.rs` - `src/librustc_mir/` - `src/tools/clippy/clippy_lints/src/consts.rs` Upon building Miri based on the new changes it was verified that no changes needed to be made with the Miri project. Related issue rust-lang#71193
For historic reasons, Miri calls uninitialized memory and the value that you get when reading it "undef(ined)". This is potentially derived from LLVM
undef
. However, that is actually a misnomer -- Miri's "undef" is much more like LLVM'spoison
. Also, the docs and user-visible message usually speak about "uninitialized memory/values", and I think that terminology makes much more sense.We should thus rename all "undef" in Miri to "uninit". In particular, but not limited to:
InvalidUndefBytes
ScalarMaybeUndef
UndefMask
Cc @oli-obk
The text was updated successfully, but these errors were encountered: