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 rustc_codegen_gcc #101104

Closed
wants to merge 3 commits into from
Closed

Conversation

ellishg
Copy link
Contributor

@ellishg ellishg commented Aug 28, 2022

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
Collaborator

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
Copy link
Collaborator

rustbot commented Aug 28, 2022

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
Copy link
Contributor

antoyo commented Aug 28, 2022

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
Copy link
Contributor Author

ellishg commented Aug 28, 2022

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
Copy link
Contributor

bors commented Aug 28, 2022

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

@antoyo
Copy link
Contributor

antoyo commented Aug 28, 2022

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
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
Copy link
Contributor

antoyo commented Aug 28, 2022

@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
Copy link
Member

bjorn3 commented Aug 30, 2022

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
Copy link
Member

bjorn3 commented Aug 30, 2022

(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
Copy link
Contributor

antoyo commented Aug 30, 2022

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
Copy link
Member

bjorn3 commented Aug 30, 2022

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
Copy link
Member

pnkfelix commented Oct 13, 2022

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
Copy link
Contributor Author

ellishg commented Oct 13, 2022

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