Skip to content

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

Merged
bors merged 2 commits intorust-lang:masterfrom
bryangarza:refactor-trait-selection-error-reporting
Apr 25, 2023
Merged

Break up long function in trait selection error reporting + clean up nearby code#110563
bors merged 2 commits intorust-lang:masterfrom
bryangarza:refactor-trait-selection-error-reporting

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
Contributor

@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
Contributor

compiler-errors commented Apr 20, 2023

One nit though

r? @compiler-errors

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

bryangarza commented Apr 20, 2023

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
Contributor

compiler-errors commented Apr 20, 2023

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

bryangarza commented Apr 20, 2023

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

bryangarza commented Apr 21, 2023

@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
Contributor

@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
Contributor

compiler-errors commented Apr 25, 2023

@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

1 more reviewer

@compiler-errors compiler-errors compiler-errors approved these changes

Reviewers whose approvals may not affect merge requirements

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