Skip to content
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

Format src/test/ui-fulldeps #98850

Closed
wants to merge 2 commits into from

Conversation

ChrisDenton
Copy link
Member

These tests are relatively special as they don't need a lot of UI-testing comments (other than a small number of error annotations). Honestly, this was easier to do than describe. A simple fmt then bless seems to have worked out ok, only needing a very few manual fixups where an ERROR was moved to the next line (see internal-lints/lint_pass_impl_without_macro.rs and lint-plugin-forbid-cmdline.rs).

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 3, 2022
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2022
@ChrisDenton
Copy link
Member Author

Hm, maybe not so easy then. Weird that it passed locally on Windows and Linux.

@rust-log-analyzer

This comment has been minimized.

@ChrisDenton ChrisDenton marked this pull request as draft July 3, 2022 18:32
@ChrisDenton ChrisDenton marked this pull request as ready for review July 3, 2022 19:59
@ChrisDenton
Copy link
Member Author

Oh duh, I just need to do a --stage 2 test otherwise some aren't run.

All the additional changes are literally just converting { } to {} or updating line numbers. So nothing "interesting" going on, which I take as a good sign. Let's see if CI agrees...

@ChrisDenton
Copy link
Member Author

Ok, so that passed.

The only question now is if this something we actually want? My argument is that this was very easy to do (stage 2 blunder aside). There were only a few places that needed manual adjustment to take in to account of the comment formatting which suggests formatting these will not be a burden. And even if these tests are eventually moved, at least they'll start off formatted in the rustc style.

Comment on lines -20 to +21
impl LintPass for Foo { //~ERROR implementing `LintPass` by hand
impl LintPass for Foo {
//~^ERROR implementing `LintPass` by hand
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and below) was the one place were I think the formatting was slightly better originally. But it's not too bad. IMHO, the best to fit in with the formatting would be if we had a way to point downward at error lines. But we don't.

fn bar<T>(f: extern "Rust" fn(&T), t: &T) { }
fn bar<T>(f: fn(&T), t: &T) {}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, hm. My eyes glanced over that initially. Removing code seems more of an actual problem even for fulldeps. I wasn't aware rustfmt did that. I'm not sure if that matters here but it's got me concerned enough to worry about doing this at all. And the issue (#18502) is from pre-1.0 Rust which makes it awkward for me to tell if this matters.

I'm fairly confident that the other tests aren't sensitive to formatting (or at least they aren't intending to be) but this is something different.

@ChrisDenton
Copy link
Member Author

Closing this as I'm no longer sure it's a good idea (though I would still like to fmt at least the stdlib tests in /src/test/ui-fulldeps).

@ChrisDenton ChrisDenton closed this Jul 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants