-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Trim runtest #89240
Trim runtest #89240
Conversation
(rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
Why this change? Rustc will happily accept |
@jyn514 More a stylistic preference than anything else. I will be redoing this PR as some smaller changes were unnecessary. |
@Nicholas-Baron it seems better to keep the original filename to avoid more churn than necessary. |
4469251
to
e885bae
Compare
☔ The latest upstream changes (presumably #89101) made this pull request unmergeable. Please resolve the merge conflicts. |
Rather than |
@camsteffen While I agree with the sentiment, the |
e885bae
to
8133f4d
Compare
I think this is not quite the change I would expect for this goal. Generally speaking, just moving code to a separate file (particularly when the move is based on a somewhat arbitrary line) isn't really making it easier to understand the file I think. Instead, I would suggest some of these changes as a start, if you'd like to cleanup compiletest a little. I think these will lead to slightly better structure for the code, while keeping the bits that have a shared structure to them in the same place for easier reading.
I suspect this set of changes is likely harder, and might not get to <3000 lines; there's likely more cleanup work possible though. This module has grown pretty organically over the last ~10 years -- there's a lot of small bits that can be adjusted to make things better. There's probably also subtle inconsistencies between various functions that would likely be nice to cleanup and unify, and filing PRs for those would be helpful -- likely some care is needed, perhaps even some small test updates, but overall a good goal. You might also be interested in #40713, as a somewhat larger project. |
@Mark-Simulacrum Thank you for the suggestions. I have spun them off as a "subtask", that will probably take multiple PRs to finish. |
Work towards #60302.
src/tools/compiletest/src/runtest.rs
was over 4k lines. To break it down into smaller chunks,TestCx
struct was extracted, as it constituted the bulk of the file.run_foo_test
were moved into a separate file, as they were ~1k lines by themselves.