Skip to content

Format rustc_codegen_gcc#101104

Closed
ellishg wants to merge 3 commits into
rust-lang:masterfrom
ellishg:lint-codegen-gcc
Closed

Format rustc_codegen_gcc#101104
ellishg wants to merge 3 commits into
rust-lang:masterfrom
ellishg:lint-codegen-gcc

Conversation

@ellishg

@ellishg ellishg commented Aug 28, 2022

Copy link
Copy Markdown
Contributor

Allow formatting and then format the rustc_codegen_gcc crate.

Since the commit to format this crate is quite large, we also add the commit hash to the .git-blame-ignore-revs file which GitHub will use for the blame UI (see https://github.blog/changelog/2022-03-24-ignore-commits-in-the-blame-view-beta/).

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 28, 2022
@rust-highfive

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot

rustbot commented Aug 28, 2022

Copy link
Copy Markdown
Collaborator

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2022
@antoyo

antoyo commented Aug 28, 2022

Copy link
Copy Markdown
Contributor

Is there any requirement for formatting rustc's external projects?
If not, I would be against this change as I prefer another formatting style.

@ellishg

ellishg commented Aug 28, 2022

Copy link
Copy Markdown
Contributor Author

Is there any requirement for formatting rustc's external projects? If not, I would be against this change as I prefer another formatting style.

Sorry I'm fairly new and I don't know about requirements. From the developer guide (https://rustc-dev-guide.rust-lang.org/conventions.html#formatting-and-the-tidy-script), developers are encouraged to run ./x.py fmt and lines should be no more than 100 characters long.

I recently opened my first rust pr #101075 and was surprised to see that my changes were not formatted.

@bors

bors commented Aug 28, 2022

Copy link
Copy Markdown
Collaborator

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

@antoyo

antoyo commented Aug 28, 2022

Copy link
Copy Markdown
Contributor

Since the CI allows ignoring the formatting check for external projects, I assumed it was okay not to format the code.
I could be wrong, though.

@Mark-Simulacrum

Copy link
Copy Markdown
Member

We currently don't format all subtrees I think, primarily because doing so requires that the rustfmt (and configuration) is consistent across both and that's currently not done.

I think in the long run it's going to be expected that all subtrees are formatted with rustfmt for the same reasons that we formatted rustc (consistency and lack of discussions around style in PRs), but I don't think there's any rush to do so, since most contributions are still expected to be made externally (in the external repo).

So I think for now we can hold off and close this PR. x.py fmt should still do the right thing for contributors and not reformat code (as should editors, presuming they respect rustfmt.toml: https://github.com/rust-lang/rust/blob/master/rustfmt.toml#L22).

@antoyo FWIW, if you have specific stylistic preferences that rustfmt could respect (e.g., options to set) it's plausible that those could be set on a per-directory basis or similar. Might be worth opening a thread on Zulip.

@antoyo

antoyo commented Aug 28, 2022

Copy link
Copy Markdown
Contributor

@antoyo FWIW, if you have specific stylistic preferences that rustfmt could respect (e.g., options to set) it's plausible that those could be set on a per-directory basis or similar. Might be worth opening a thread on Zulip.

I tried to setup a rustfmt config in the past, but there were many missing options that were missing to get the formatting I wanted.

@bjorn3

bjorn3 commented Aug 30, 2022

Copy link
Copy Markdown
Member

If we end up formatting cg_gcc I think this PR should be opened on https://github.com/rust-lang/rustc_codegen_gcc/ if we end up doing this and after it is merged the subtree in this repo should immediately be updated to avoid a huge amount of conflicts.

@bjorn3

bjorn3 commented Aug 30, 2022

Copy link
Copy Markdown
Member

(as should editors, presuming they respect rustfmt.toml: https://github.com/rust-lang/rust/blob/master/rustfmt.toml#L22).

When using the config suggested by the rustc dev guide which has format on save enabled, cg_gcc gets automatically formatted by vscode. I believe rustfmt doesn't respect the ignore field when getting input from stdin. Not that it actually can respect it I think.

@antoyo

antoyo commented Aug 30, 2022

Copy link
Copy Markdown
Contributor

When using the config suggested by the rustc dev guide which has format on save enabled, cg_gcc gets automatically formatted by vscode. I believe rustfmt doesn't respect the ignore field when getting input from stdin. Not that it actually can respect it I think.

I've been suggested to add this file to prevent the formatting. Do you mean it doesn't work in VS Code?

@bjorn3

bjorn3 commented Aug 30, 2022

Copy link
Copy Markdown
Member

It works for cargo fmt, but not for rustfmt invoked through rust-analyzer. Maybe rust-analyzer needs to spawn rustfmt in the same directory as file to be formatted for it to work?

@pnkfelix

pnkfelix commented Oct 13, 2022

Copy link
Copy Markdown
Contributor

Discussed in T-compiler meeting. I do not think we should plan to land this PR. As discussed in this comment thread, rustc_codegen_gcc is an external project and may not want to adhere to the rules that rustc uses for its source code. We have mechanisms for allowing that (namely, .rustfmt.toml), but those mechanisms are not working in key contexts like rust-analyzer, probably due to rust-lang/rust-analyzer#10826

@ellishg

ellishg commented Oct 13, 2022

Copy link
Copy Markdown
Contributor Author

Discussed in T-compiler meeting. I do not think we should plan to land this PR. As discussed in this comment thread, rustc_codegen_gcc is an external project and may not want to adhere to the rules that rustc uses for its source code. We have mechanisms for allowing that (namely, .rustfmt.toml), but those mechanisms are not working in key contexts like rust-analyzer, probably due to rust-lang/rust-analyzer#10826

This makes sense to me. I will close this now.

@ellishg ellishg closed this Oct 13, 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.

9 participants