Run rustfmt #1118

oli-obk · 2019-12-21T10:54:26Z

This is cargo +nightly fmt --all with rustc 1.41.0-nightly (c8ea4ace9 2019-12-14)

benches/fibonacci.rs

src/bin/cargo-miri.rs

src/bin/miri-rustc-tests.rs

src/intptrcast.rs

src/machine.rs

src/operator.rs

src/shims/intrinsics.rs

src/stacked_borrows.rs

RalfJung · 2019-12-21T11:49:03Z

This basically confirms what I thought about rustfmt: that a large fraction of the things it changes are somewhat questionable, and a smaller fraction outright makes things worse and needs manual clean-up. :/

oli-obk · 2019-12-21T11:57:57Z

a large fraction of the things it changes are somewhat questionable

But automatable :D I'm fine with a few things being odd

and a smaller fraction outright makes things worse

I think most of these I have been able to fix with the rustfmt.toml flag

and needs manual clean-up.

Well, the situations I have manually fixed weren't ideal to begin with. I think some of them are now significantly better

benches/smoke.rs

src/bin/miri-rustc-tests.rs

oli-obk · 2019-12-21T13:22:51Z

src/shims/foreign_items.rs

+            | "acos"
+            | "asin"
+            | "atan"
+            => {


opinions on this style? I like it because it makes changing the list never cause fallout in unrelated elements

Not all lines are consistent though, the first line is different. So the thing that would be most like how we format comma-separated lists would be to make every line "foo" |. Unfortunately though, this does not compile.

So the version taht compiles would be to make every line | "foo", I guess? Including the first line, like this.

Looks like that's what you got. So in the confines of the Rust grammar, I guess that is the best we'll get.

Shame that rustfmt destroys that. :/

src/shims/foreign_items.rs

oli-obk · 2019-12-22T08:58:06Z

Do you want to wait on merging this until the second rustfmt issue is solved or are the rustfmt::skips ok for now?

RalfJung · 2019-12-22T10:34:17Z

No, we don't have to wait for that.

But we should probably land #1101 first due to conflicts.

RalfJung · 2019-12-22T11:18:21Z

Also it would probably be best to exclude env.rs to avoid conflicts with #1098.

bors · 2019-12-22T17:20:58Z

☔ The latest upstream changes (presumably #1101) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung · 2019-12-23T10:27:25Z

src/helpers.rs

+                _ => {
+                    throw_unsup_format!("The {} error cannot be transformed into a raw os error", e)
+                }


Maybe we could set match_arm_blocks = false to avoid things like this?

Interestingly this still gets a block, even with match_arm_blocks = false? Any idea why?

maybe rustfmt bailed out further up in the ast here out of other reasons?

RalfJung · 2019-12-23T10:30:27Z

rustc's rustfmt.toml also has version = "Two". Should we use that, too? Not sure what difference it makes.

oli-obk · 2019-12-23T10:59:20Z

no clue about version = "Two", let's see what happens

oli-obk · 2019-12-23T11:22:48Z

version = "Two" and match_arm_blocks = false have superb effects on the code.

oli-obk · 2019-12-23T11:23:46Z

I'll squash and exclude env.rs changes after you're alright with the two new commits

RalfJung · 2019-12-23T11:43:47Z

Works for me. I'd still format some bits differently but those are nits.

oli-obk · 2019-12-23T11:56:47Z

@bors r+

bors · 2019-12-23T11:56:48Z

📌 Commit 7ead530 has been approved by oli-obk

bors · 2019-12-23T11:56:55Z

⌛ Testing commit 7ead530 with merge 20e843f...

Run rustfmt This is `cargo +nightly fmt --all` with `rustc 1.41.0-nightly (c8ea4ace9 2019-12-14)`

oli-obk · 2019-12-23T11:57:10Z

next step, block CI on rustfmt?

RalfJung · 2019-12-23T12:01:42Z

I feared you'd suggest this. :/ But that seems to be the way the Rust community is moving, even though I am not terribly happy about it TBH. I don't consider rustfmt "better" unambiguously enough.

Maybe let's wait a bit how enforcing rustfmt works out for rustc itself?

bors · 2019-12-23T12:15:37Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 20e843f to master...

oli-obk · 2019-12-23T13:38:56Z

I feared you'd suggest this. :/ But that seems to be the way the Rust community is moving, even though I am not terribly happy about it TBH. I don't consider rustfmt "better" unambiguously enough.

I don't consider rustfmt better either. But it's just so convenient when I can turn it on on a project. I stop doing any kind of formatting myself. I press save or move the window focus and the file is formatted.

Maybe let's wait a bit how enforcing rustfmt works out for rustc itself?

Fine by me. We can also leave it off for this repo since you dislike enforcing rustfmt on CI. It just means that sometimes we'll start getting unrelated changes if someone with rustfmt turned on edits something that was previously edited by someone who doesn't use rustfmt.

An intermediate state would be to add CI trickery to automatically add a rustfmt commit before running bors