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

Split rustc_typeck::check into separate files #76906

Merged
merged 13 commits into from
Sep 22, 2020

Conversation

Nicholas-Baron
Copy link
Contributor

Contributing to #60302.

@rust-highfive
Copy link
Collaborator

r? @oli-obk

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Sep 19, 2020

I'm not a big fan of util modules. Can you try to find things that can be placed in new modules for any additional changes you do?

@Nicholas-Baron
Copy link
Contributor Author

Sure. Could it be a one struct to one mod generally, with some exceptions?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 19, 2020

Well, if anything is reasonably groupable, then that is definitely preferrable. Ideally instead of a util module, the things that are in the util module live in the main check module, and we'd move out large chunks of things instead.

@Nicholas-Baron
Copy link
Contributor Author

@oli-obk Is something like what I did for GatherLocalsVisitor acceptable?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 19, 2020

perfect!

@Nicholas-Baron
Copy link
Contributor Author

It seems that half of the mod.rs file was dedicated to the FnCtxt struct. I have moved the FnCtxt struct and its related impls to a separate file that is now over 3000 lines.

mod.rs is fine, for now (~2800 lines).

@Nicholas-Baron
Copy link
Contributor Author

I have gotten mod.rs down to ~1150 lines. @oli-obk, should I continue to put more parts in their own files or it just over a thousand lines good enough?

Additionally, fn_ctxt.rs is now throwing file length errors. It is about 200 lines over the limit, but I do not know what to move to other files.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 20, 2020

Additionally, fn_ctxt.rs is now throwing file length errors. It is about 200 lines over the limit, but I do not know what to move to other files.

I guess let's mark that file as oversized for now and see how to split it later?

@bors
Copy link
Contributor

bors commented Sep 20, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@Nicholas-Baron
Copy link
Contributor Author

@oli-obk so the issue is no longer that mod.rs is too long, but that the FnCtxt struct has too much functionality attached to it.

How should I format the //TODO that is going to be under the // ignore-tidy-filelength for fn_ctxt.rs?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 21, 2020

How should I format the //TODO that is going to be under the // ignore-tidy-filelength for fn_ctxt.rs?

I'm not sure I understand? Just a message about "try to figure out how to split this file into parts" should be sufficient

Also, don't use TODO, CI will complain about that, use FIXME

@Nicholas-Baron
Copy link
Contributor Author

I am asking about the format, as I have seen FIXMEs with some attribution to an author. However, some seem to not have an attribution, so I stand corrected.

@oli-obk
Copy link
Contributor

oli-obk commented Sep 21, 2020

Oh heh, that's just convention if you want others to bug you about that FIXME

@oli-obk
Copy link
Contributor

oli-obk commented Sep 21, 2020

Ok, so let's move this out of draft and then I'll do a review and give the PR priority to avoid breakage

@Nicholas-Baron Nicholas-Baron marked this pull request as ready for review September 21, 2020 16:52
@Nicholas-Baron
Copy link
Contributor Author

@oli-obk ready for review

@oli-obk
Copy link
Contributor

oli-obk commented Sep 22, 2020

@bors r+ p=5

@bors
Copy link
Contributor

bors commented Sep 22, 2020

📌 Commit ccd218d has been approved by oli-obk

@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 22, 2020
@bors
Copy link
Contributor

bors commented Sep 22, 2020

⌛ Testing commit ccd218d with merge c113030...

@bors
Copy link
Contributor

bors commented Sep 22, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing c113030 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants