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.

Sign up for GitHub

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

Jump to bottom

Migrate rustc_codegen_gcc to SessionDiagnostics #101075

Merged
merged 16 commits into from
Sep 30, 2022
Merged

Migrate rustc_codegen_gcc to SessionDiagnostics #101075

merged 16 commits into from
Sep 30, 2022

Conversation

ellishg
Copy link
Contributor

@ellishg ellishg commented Aug 27, 2022

As part of #100717 this pr migrates diagnostics to SessionDiagnostics for the rustc_codegen_gcc crate.

@rustbot label +A-translation

@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 @nagisa (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2022

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 27, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 27, 2022
@ellishg ellishg force-pushed the rustc_codegen_gcc_diagnostics branch from 24a7f55 to 913350e Compare August 27, 2022 04:26
@ellishg
Copy link
Contributor Author

ellishg commented Aug 27, 2022

The diagnostics I'm missing come from
https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/context.rs#L478-L484

I saw something similar in rustc_codegen_llvm but #101005 has not finished yet.

I also found some calls to span_invalid_monomorphization_error() that I suspect need to be changed.

https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/intrinsic/simd.rs#L27-L30
https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/intrinsic/simd.rs#L453-L456

These were not caught by #![deny(...)] and I'm not sure if that means they don't need to be translated, or if this is an edge case.

@ellishg ellishg mentioned this pull request Aug 28, 2022
Closed
@nagisa
Copy link
Member

nagisa commented Aug 29, 2022

r? @compiler-errors / @davidtwco

@davidtwco
Copy link
Member

The diagnostics I'm missing come from

https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/context.rs#L478-L484

I saw something similar in rustc_codegen_llvm but #101005 has not finished yet.

I feel like LayoutError could basically implement SessionDiagnostic, maybe we could implement it for Spanned<LayoutError> (Spanned)?

I also found some calls to span_invalid_monomorphization_error() that I suspect need to be changed.

https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/intrinsic/simd.rs#L27-L30

https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/intrinsic/simd.rs#L453-L456

These were not caught by #![deny(...)] and I'm not sure if that means they don't need to be translated, or if this is an edge case.

These should be translated, and ideally the lint would fire on them, I've added a link to this to #100717 so someone can go fix that.

@davidtwco davidtwco mentioned this pull request Aug 30, 2022
Open
84 tasks
davidtwco
davidtwco reviewed Aug 30, 2022
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

This is a great start, thanks!

compiler/rustc_codegen_gcc/src/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_gcc/src/errors.rs Outdated Show resolved Hide resolved
@ellishg ellishg force-pushed the rustc_codegen_gcc_diagnostics branch from 6c8fa90 to 5765171 Compare September 1, 2022 22:25
@davidtwco
Copy link
Member

This is great progress, I'd like it if you could address the feedback from my earlier comment (#101075 (comment)) :)

@ellishg
Copy link
Contributor Author

ellishg commented Sep 3, 2022

The diagnostics I'm missing come from
https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/context.rs#L478-L484

I saw something similar in rustc_codegen_llvm but #101005 has not finished yet.

I feel like LayoutError could basically implement SessionDiagnostic, maybe we could implement it for Spanned<LayoutError> (Spanned)?

I tried to implement SessionDiagnostic for LayoutError and Spanned<T>, but I ended up getting lifetime errors. I don't quite understand it, but I think it was related to 'gcc and tcx being different lifetimes.

I also found some calls to span_invalid_monomorphization_error() that I suspect need to be changed.
https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/intrinsic/simd.rs#L27-L30

https://github.com/rust-lang/rust/blob/913350ecaf6a0fc621c6c43f0764956e1c47cc15/compiler/rustc_codegen_gcc/src/intrinsic/simd.rs#L453-L456

These were not caught by #![deny(...)] and I'm not sure if that means they don't need to be translated, or if this is an edge case.

These should be translated, and ideally the lint would fire on them, I've added a link to this to #100717 so someone can go fix that.

I'm still working on this. I think I need to create a new function similar to span_invalid_monomorphization_error() that takes a SessionDiagnostic. I also realized there are lots of other untranslatable strings in simd.rs that I would need to fix.

@davidtwco
Copy link
Member

I tried to implement SessionDiagnostic for LayoutError and Spanned<T>, but I ended up getting lifetime errors. I don't quite understand it, but I think it was related to 'gcc and tcx being different lifetimes.

If you share the code from your attempt then maybe we can work this out.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ellishg ellishg force-pushed the rustc_codegen_gcc_diagnostics branch from 655c3fa to 1e3d953 Compare September 12, 2022 01:12
@ellishg ellishg force-pushed the rustc_codegen_gcc_diagnostics branch from 1e3d953 to 22dbcb0 Compare September 13, 2022 21:21
@bors

This comment was marked as resolved.

Sign in to view
@davidtwco
Copy link
Member

Apologies for the delay in responding to this, replied to the pending comments now.

@ellishg ellishg force-pushed the rustc_codegen_gcc_diagnostics branch from 22dbcb0 to 5c7e629 Compare September 24, 2022 18:09
@ellishg
Copy link
Contributor Author

ellishg commented Sep 24, 2022

I've finally figured out the lifetime error. Thanks @davidtwco for all your help! I believe I've migrated all the diagnostics so this is ready for review!

antoyo
antoyo reviewed Sep 24, 2022
View reviewed changes
Copy link
Contributor

@antoyo antoyo left a comment

Choose a reason for hiding this comment

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

LGTM

compiler/rustc_codegen_gcc/src/intrinsic/simd.rs Outdated Show resolved Hide resolved
davidtwco
davidtwco reviewed Sep 26, 2022
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks great, left a few comments

compiler/rustc_codegen_gcc/src/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_gcc/src/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_gcc/src/errors.rs Outdated Show resolved Hide resolved
compiler/rustc_error_messages/locales/en-US/codegen_gcc.ftl Outdated Show resolved Hide resolved
@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 29, 2022

📌 Commit 01439c9 has been approved by davidtwco

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 30, 2022
@matthiaskrgr
Rollup merge of rust-lang#101075 - ellishg:rustc_codegen_gcc_diagnost…
4684e21 
…ics, r=davidtwco

Migrate rustc_codegen_gcc to SessionDiagnostics

As part of rust-lang#100717 this pr migrates diagnostics to `SessionDiagnostics` for the `rustc_codegen_gcc` crate.

`@rustbot` label +A-translation
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 30, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 30, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 30, 2022
@bors
Auto merge of rust-lang#102509 - matthiaskrgr:rollup-gtenet8, r=matth…
f914b82 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#101075 (Migrate rustc_codegen_gcc to SessionDiagnostics )
 - rust-lang#102350 (Improve errors for incomplete functions in struct definitions)
 - rust-lang#102481 (rustdoc: remove unneeded CSS `.rust-example-rendered { position }`)
 - rust-lang#102491 (rustdoc: remove no-op source sidebar `opacity`)
 - rust-lang#102499 (Adjust the s390x data layout for LLVM 16)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 25017f8 into rust-lang:master Sep 30, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 30, 2022
@ellishg ellishg deleted the rustc_codegen_gcc_diagnostics branch October 1, 2022 15:47
antoyo pushed a commit to antoyo/rust that referenced this pull request Jun 19, 2023
@matthiaskrgr
Rollup merge of rust-lang#101075 - ellishg:rustc_codegen_gcc_diagnost…
869203c 
…ics, r=davidtwco

Migrate rustc_codegen_gcc to SessionDiagnostics

As part of rust-lang#100717 this pr migrates diagnostics to `SessionDiagnostics` for the `rustc_codegen_gcc` crate.

``@rustbot`` label +A-translation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@compiler-errors compiler-errors compiler-errors left review comments

@antoyo antoyo antoyo left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@compiler-errors compiler-errors

Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.
9 participants
@ellishg @rust-highfive @rustbot @nagisa @davidtwco @rust-log-analyzer @bors @antoyo @compiler-errors