`rustc_mir_transform` cleanups 3 #130175

nnethercote · 2024-09-09T23:23:35Z

More cleanups in the style of #129929.

By reflowing comment lines that are too long, and a few that are very short. Plus some other very minor formatting tweaks.

These are all functions with a single callsite, where having a separate function does nothing to help with readability. These changes make the code a little shorter and easier to read.

The "as" is equivalent to "because", but I originally read it more like "when", which was confusing.

This was non-obvious to me.

In all cases the struct can own the relevant thing instead of having a reference to it. This makes the code simpler, and in some cases removes a struct lifetime.

It's a thin wrapper around `check_live_drops`, but it's enough to fix the FIXME comment.

rustbot · 2024-09-09T23:23:44Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

saethlin · 2024-09-10T00:02:29Z

compiler/rustc_mir_transform/src/add_retag.rs

+                            // need to also add a retag statement. *Except* if we are deref'ing a
+                            // Box, because those get desugared to directly working with the inner
+                            // raw pointer! That's relevant for `RawPtr` as Miri otherwise makes it


My kingdom for a code review tool that can ignore this diff

saethlin · 2024-09-10T00:05:47Z

compiler/rustc_mir_transform/src/gvn.rs

+        // Clone dominators because we need them while mutating the body.
+        let dominators = body.basic_blocks.dominators().clone();
+
+        let mut state = VnState::new(tcx, body, param_env, &ssa, dominators, &body.local_decls);


What happened here? How did rustfmt let the original code be?

I don't see what's wrong with the original code...

Upon further inspection, I don't either. The way "remove whitespace" displayed this part of the diff confused me.

saethlin · 2024-09-10T00:19:26Z

compiler/rustc_mir_transform/src/lib.rs

            None,
        );
-        check_consts::post_drop_elaboration::check_live_drops(tcx, body); // FIXME: make this a MIR lint


pm::run_passes has a bit of extra logic in it for running MIR linting/validation as well as -Zmir-enable-passes. So this isn't necessarily a no-op, but I'm down with following the FIXME.

I think this is just a comment, not a request for a change, right?

Yeah. Just a comment.

saethlin · 2024-09-10T00:32:37Z

I have a question (and two comments, such as they are) ^
@rustbot author

Zalathar · 2024-09-10T01:38:00Z

compiler/rustc_mir_transform/src/copy_prop.rs

+            // A move out of a projection of a copy is equivalent to a copy of the original
+            // projection.


Nitpick: These single-word reflows are a bit awkward.

For single-line comments that don't quite fit in 100 columns, I'd consider breaking them even earlier (closer to 80 columns) just so that the second line is a bit longer.

(But if you're using automation to do this then maybe it's not worth the extra effort. 🤷‍♂️)

I just reflowed them all to width 100, because that's the closest I could get to the original while observing the "don't exceed 100 chars" rule. I definitely don't want to be thinking about different widths for different comments depending on their contents. One word on a line by itself is fine.

saethlin · 2024-09-10T13:26:14Z

@bors r+

bors · 2024-09-10T13:26:17Z

📌 Commit 8949b44 has been approved by saethlin

It is now in the queue for this repository.

…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#130143 (miri-test-libstd: add missing BOOTSTRAP_ARGS) - rust-lang#130173 (rustdoc: add two regression tests) - rust-lang#130175 (`rustc_mir_transform` cleanups 3) - rust-lang#130184 (coverage: Clean up terminology in counter creation) - rust-lang#130185 (abi/compatibility test: remove tests inside repr(C) wrappers) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#130175 - nnethercote:rustc_mir_transform-cleanups-3, r=saethlin `rustc_mir_transform` cleanups 3 More cleanups in the style of rust-lang#129929. r? `@saethlin`

nnethercote added 8 commits September 10, 2024 08:42

Improve comment formatting.

8235af0

By reflowing comment lines that are too long, and a few that are very short. Plus some other very minor formatting tweaks.

Inline and remove some functions.

48064d4

These are all functions with a single callsite, where having a separate function does nothing to help with readability. These changes make the code a little shorter and easier to read.

Clarify a comment.

baa16d2

The "as" is equivalent to "because", but I originally read it more like "when", which was confusing.

Add a useful comment.

e26692d

Add a useful comment about PromoteTemps.

51e1c39

This was non-obvious to me.

Use IndexVec::from_raw to construct a const IndexVec.

d1c55a3

Remove references from some structs.

7023402

In all cases the struct can own the relevant thing instead of having a reference to it. This makes the code simpler, and in some cases removes a struct lifetime.

Make check_live_drops into a MirLint.

8949b44

It's a thin wrapper around `check_live_drops`, but it's enough to fix the FIXME comment.

rustbot assigned saethlin Sep 9, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 9, 2024

saethlin reviewed Sep 10, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 10, 2024

Zalathar reviewed Sep 10, 2024

View reviewed changes

fmease changed the title ~~rustc_mir_transformcleanups 3~~ rustc_mir_transform cleanups 3 Sep 10, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 10, 2024

matthiaskrgr mentioned this pull request Sep 10, 2024

Rollup of 5 pull requests #130200

Merged

bors merged commit accd77e into rust-lang:master Sep 10, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Sep 10, 2024

nnethercote deleted the rustc_mir_transform-cleanups-3 branch September 11, 2024 06:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rustc_mir_transform` cleanups 3 #130175

`rustc_mir_transform` cleanups 3 #130175

nnethercote commented Sep 9, 2024

rustbot commented Sep 9, 2024

saethlin Sep 10, 2024

saethlin Sep 10, 2024

nnethercote Sep 10, 2024

saethlin Sep 10, 2024

saethlin Sep 10, 2024

nnethercote Sep 10, 2024

saethlin Sep 10, 2024

saethlin commented Sep 10, 2024 •

edited

Loading

Zalathar Sep 10, 2024

nnethercote Sep 10, 2024

saethlin commented Sep 10, 2024

bors commented Sep 10, 2024

		// A move out of a projection of a copy is equivalent to a copy of the original
		// projection.

rustc_mir_transform cleanups 3 #130175

rustc_mir_transform cleanups 3 #130175

Conversation

nnethercote commented Sep 9, 2024

rustbot commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Sep 10, 2024

bors commented Sep 10, 2024

`rustc_mir_transform` cleanups 3 #130175

`rustc_mir_transform` cleanups 3 #130175

saethlin commented Sep 10, 2024 •

edited

Loading