`rustc_mir_transform` cleanups 3 by nnethercote · Pull Request #130175 · rust-lang/rust

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

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

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

Uh oh!

Conversation

nnethercote commented Sep 9, 2024

Uh oh!

rustbot commented Sep 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saethlin commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saethlin commented Sep 10, 2024

Uh oh!

bors commented Sep 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

saethlin commented Sep 10, 2024 •

edited

Loading