Miri rename undef uninit #74664
Miri rename undef uninit #74664
Conversation
Renamed the function ScalarMaybeUninit::not_undef to ScalarMaybeUninit::check_init in the file src/librustc_middle/mir/interpret/value.rs, to reflect changes in terminology used. Related issue #71193
| @@ -565,7 +565,7 @@ impl<'tcx, Tag: Copy, Extra> Allocation<Tag, Extra> { | |||
| }) | |||
| } | |||
|
|
|||
| pub fn mark_definedness(&mut self, ptr: Pointer<Tag>, size: Size, new_state: bool) { | |||
| pub fn mark_init(&mut self, ptr: Pointer<Tag>, size: Size, new_state: bool) { | |||
There was a problem hiding this comment.
new_state is not very clear; maybe is_init or so?
There was a problem hiding this comment.
The latter does make more sense.
|
This looks great apart from one nit. :) |
|
Looking great, thanks. :) @pnadon do you think you will have time to fix https://github.com/rust-lang/miri/ as well? Let me know if you need any help with that. |
|
📌 Commit f8c5722f9d662980ed7a3895e94741af8842e38e has been approved by |
bors
added
the
S-waiting-on-bors
|
@bors rollup |
Do you mean the changes referenced in this comment? If so I would be glad to! |
|
@pnadon: 🔑 Insufficient privileges: Not in reviewers |
I assume you may have been referring to the renames in |
No I mean that Miri (the standalone tool at https://github.com/rust-lang/miri/, not the Miri core engine which this PR touches) is using some of the APIs you renamed here, and will fail to build once this PR lands. Someone has to get it to build again. :) |
Oh I see, yes I'll look into that! |
|
As for the remaining renames mentioned in #71193, that is an orthogonal question, feel free to continue working on those as well. :) Beyond what is listed in #71193 (comment), I have not yet taken stock what even needs to be done. |
|
Actually, would you want me to finish the remaining renames in |
|
Miri should always build, so once this lands a Miri PR needs to be prepared ASAP. It would be even better if you could prepare that Miri PR before this one here lands. :D |
That makes sense, since this morning I have also renamed the 3 other functions / structs mentioned in that comment, I guess what I was trying to say was that I could commit them here, and then start working on https://github.com/rust-lang/miri/, once I get the changes made in the Miri repo, I could open a PR there and then we could close both simultaneously. Does that sound good? |
|
Oh sure, we can add more renames to this PR. |
bors
added
S-waiting-on-author
|
I ripgrep'd through Miri's src and only found ScalarMaybeUninit::not_undef, as well as a function call |
|
i also ripgrep'd the other changes I just made in Miri, and I could not find them. This seems strange to me, but I will open a PR on Miri with what I have so far and continue my work there. |
|
The instructions at https://github.com/rust-lang/miri/blob/master/CONTRIBUTING.md#building-miri-with-a-locally-built-rustc explain how you can build-test Miri against your locally built rustc. Make sure you use a recent rustc master though to avoid build failures due to unrelated changes. You can skip the |
|
Ok, aside from the comments though I'm assuming everything looks ok. In that case, I will work on getting Miri to build, and also fix the above comments. |
|
Miri seems to build correctly, and I have changed the above comments. |
| initial: bool, | ||
| /// The lengths of ranges that are run-length encoded. | ||
| /// The definedness of the ranges alternate starting with `initial`. | ||
| /// The initialization state of the ranges alternate starting with `initial`. |
There was a problem hiding this comment.
| /// The initialization state of the ranges alternate starting with `initial`. | |
| /// The initialization state of the ranges alternates starting with `initial`. |
| // So if `ranges.len() > 1` then the second block is an initialized range. | ||
| !self.initial && self.ranges.len() == 1 | ||
| } | ||
| } | ||
|
|
||
| /// Transferring the definedness mask to other allocations. | ||
| /// Transferring the initialization mask to other allocations. | ||
| impl<Tag, Extra> Allocation<Tag, Extra> { | ||
| /// Creates a run-length encoding of the undef mask. |
| /// | ||
| /// Returns `Ok(())` if it's defined. Otherwise returns the range of byte | ||
| /// Returns `Ok(())` if it's initialized. Otherwise returns the range of byte | ||
| /// indexes of the first contiguous undefined access. |
There was a problem hiding this comment.
I searched again through allocation.rs and found this comment as well as one other and a variable, which were all changed appropriately. I left comments which were for other functions / parts of the code which have yet to be changed (eg. inspect_with_undef_and_ptr_outside_interpreter).
|
There are still two unresolved comments left (you can see them by scrolling through https://github.com/rust-lang/rust/pull/74664/files) |
I had left some of them since they were related to functions which still had "undef" within their name (and are currently being discussed in the related issue) to try and group the changes in the comments with the changes in the actual code, however since many of them still use the functions which have been changed, I believe it was actually better to do the rest of the comments in allocation.rs anyways (which I did in the latest commit). As for renaming the rest of the functions, I think I will hold off on that until we have decided on the new names in the issue, and do them all at once for both the Rust and Miri respositories. |
|
Yes this is great. :) Could you squash the commits into one (or a few; 12 seems a bit too much)? Then I'll r+. |
renamed Allocation::check_defined_and_ptr to Allocation::check_init_and_ptr renamed Allocation::check_defined_and_ptr to Allocation::check_init_and_ptr in src/librustc_middle/mir/interpret/allocation.rs renamed Allocation::is_defined and Allocation::check_defined, fixed documentation renamed Allocation::is_defined and Allocation::check_defined to is_init and check_init respectively. Fixed documentation so it correctly refers to "initialization" instead of "defined"-ness renamed Allocation::mark_definedness renamed Allocation::mark_definedness to Allocation::mark_init Renamed new_state parameter in Allocation::mark_init Renamed new_state to is_init, as the latter is more descriptive. renamed functions in AllocationDefinedness renamed AllocationDefinedness::all_bytes_undef and AllocationDefinedness::mark_compressed_undef_range to no_bytes_init and mark_compressed_init_range respectively. renamed AllocationDefinedness to InitMaskCompressed renamed Immediate::to_scalar_or_undef renamed to to_scalar_or_uninit fixed comment references to "undef" Changed comments referring to "undef" and "definedness" to "initialization" and "initialization state" in src/librustc_mir/interpret/memory.rs and src/librustc_middle/mir/interpret/allocation.rs changed references to "undef" in comments and a variable Changed some comments referring to "undef" to use "uninitialized" instead. Also changed a variable from "undef_end" to "uninit_end". All changes were made within src/librustc_middle/mir/interpret/allocation.rs. Changed more comments referring to undef Changed comments to use "uninitialized" instead of "undef" in src/librustc_middle/mir/interpret/allocation.rs.
|
Squashed into 2 commits |
|
Thanks. :) @bors r+ |
|
📌 Commit ef9c4f5 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions, checks-azure |
|
Thank you @RalfJung for your patience and help throughout this, this was my first contribution to Rust and I really appreciated the time and effort you put into making sure I did things correctly. |
|
@pnadon thanks for contributing, here's hoping that you'll continue to enjoy working on Rust. :) |
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`
Renamed parts of code within the
librustc_middle/mir/interpret/directory.Related issue #71193