Skip to content

Break up long function in trait selection error reporting + clean up nearby code #110563

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
Merged
merged 2 commits into from
Apr 25, 2023
Merged

Break up long function in trait selection error reporting + clean up nearby code #110563

merged 2 commits into from
Apr 25, 2023

Conversation

bryangarza
Copy link
Contributor

@bryangarza bryangarza commented Apr 19, 2023 •
edited
Loading

  • Move blocks of code into their own functions
  • Replace a few function argument types with their type aliases
  • Create "AppendConstMessage" enum to replace a nested Option.

@rustbot
Copy link
Collaborator

rustbot commented Apr 19, 2023

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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. labels Apr 19, 2023
compiler-errors
compiler-errors approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Just noting that a title like "Break up long function in trait selection error reporting" suggest that this PR is doing more than just splitting out one helper function from error reporting, so I wouldn't consider that function's size problem to have been vanquished just yet.

But there's nothing necessarily wrong with this impl, so r=me

@compiler-errors
Copy link
Member

One nit though

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned jackh726 Apr 20, 2023
@bryangarza
Copy link
Contributor Author

Just noting that a title like "Break up long function in trait selection error reporting" suggest that this PR is doing more than just splitting out one helper function from error reporting, so I wouldn't consider that function's size problem to have been vanquished just yet.

Good point, I’ll rename it to be more specific. I do plan on putting up more changes (or maybe I should do it all in one go in this PR).

@compiler-errors
Copy link
Member

I do plan on putting up more changes (or maybe I should do it all in one go in this PR).

No preference I guess. If you do plan on working more on this PR, I'd mark this one as waiting-on-author just to make it clear it's not looking for a review yet.

@bryangarza
Copy link
Contributor Author

I do plan on putting up more changes (or maybe I should do it all in one go in this PR).

No preference I guess. If you do plan on working more on this PR, I'd mark this one as waiting-on-author just to make it clear it's not looking for a review yet.

Sounds good, I’ll just update this same PR tomorrow then

@rustbot label +S-waiting-on-author

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 20, 2023
@compiler-errors compiler-errors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 20, 2023
@bryangarza bryangarza force-pushed the refactor-trait-selection-error-reporting branch from 52f1e4a to 035a1b9 Compare April 21, 2023 03:48
@bryangarza
Copy link
Contributor Author

bryangarza commented Apr 21, 2023 •
edited
Loading

Okay, I split it up a lot more :) I tried to keep most of the if checks in the main function itself, but move the code that inserts the errors/notes etc as their own functions, that way the logic can still be mostly read top to bottom.

@bryangarza
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2023
@bors
Copy link
Collaborator

bors commented Apr 21, 2023

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

@bryangarza
Break up long function in trait selection error reporting
d0d40d2 
- Move blocks of code into their own functions
- Replace a few function argument types with their type aliases
@bryangarza bryangarza force-pushed the refactor-trait-selection-error-reporting branch from 035a1b9 to d0d40d2 Compare April 21, 2023 16:04
@bryangarza
Create "AppendConstMessage" enum
55e5a1d 
This patch creates an enum to replace a nested `Option`.
@bryangarza bryangarza force-pushed the refactor-trait-selection-error-reporting branch from 722fc88 to 55e5a1d Compare April 21, 2023 21:07
@bryangarza bryangarza changed the title Break up long function in trait selection error reporting Break up long function in trait selection error reporting + cleanup nearby code Apr 21, 2023
@bryangarza bryangarza changed the title Break up long function in trait selection error reporting + cleanup nearby code Break up long function in trait selection error reporting + clean up nearby code Apr 21, 2023
compiler-errors
compiler-errors approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Maybe this could use some additional simplification but this is a great start. Thanks!

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 25, 2023

📌 Commit 55e5a1d has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 25, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Apr 25, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2023
@bors
Auto merge of rust-lang#110789 - matthiaskrgr:rollup-92e764u, r=matth…
ed3ddb3 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#110563 (Break up long function in trait selection error reporting + clean up nearby code)
 - rust-lang#110755 ([LLVM17] Adapt to `ExplicitEmulatedTLS` removal.)
 - rust-lang#110775 (Update books)
 - rust-lang#110779 (configure.py: add flag for riscv{64,32}gc musl-root)
 - rust-lang#110782 (Revert panic oom)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5f33a8c into rust-lang:master Apr 25, 2023
@rustbot rustbot added this to the 1.71.0 milestone Apr 25, 2023
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 approved these changes

Assignees

@compiler-errors compiler-errors

Labels
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.71.0
Development

Successfully merging this pull request may close these issues.
5 participants
@bryangarza @rustbot @compiler-errors @bors @jackh726