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

Moved the main impl for FnCtxt to its own file. #77808

Merged
merged 5 commits into from
Oct 14, 2020

Conversation

Nicholas-Baron
Copy link
Contributor

Resolves #77085 without breaking the API of the FnCtxt struct.

This is a solution to the file length being over 3000 (see issue #60302).

The other solution to the file length is

  1. to change the API of this struct by
  2. encapulating certain fields of the struct into other structs.

This is a solution to the file length being over 3000, something Clippy has a problem with.

The other solution to the file length is
1. to change the API of this struct by
2. encapulating certain fields of the struct into other structs.
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Oct 11, 2020
@jyn514 jyn514 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 11, 2020
@jyn514
Copy link
Member

jyn514 commented Oct 11, 2020

@Nicholas-Baron another possible solution is to split the 'main' impl into several smaller impls, each of which has its own file. I think I prefer that in the long run since the new file here is already almost 3000 lines.

@Nicholas-Baron
Copy link
Contributor Author

@jyn514 Something like fn_ctxt_prefix where prefix is some common prefix on a function?
Example: fn_ctxt_suggestions.rs for functions starting with suggest_, fn_ctxt_checks.rs for functions starting with check_

@jyn514
Copy link
Member

jyn514 commented Oct 11, 2020

Sure, seems reasonable. I've never actually used this code before.

@Nicholas-Baron
Copy link
Contributor Author

@jyn514 The fn_ctxt_impl.rs is now just under 1,500 lines, half of the limit. This should be sustainable.

mod checks;
mod suggestions;

pub use _impl::*;
Copy link
Member

Choose a reason for hiding this comment

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

_impl is a bit of an odd naming convention - I think you picked it because it has the main impl FnCtxt. I'm not sure a better name for it though ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The pattern comes from the _match.rs file in the directory above it. Seeing that, my assumption is that Rust would not allow keywords as identifiers (a reasonable assumption).
Yes, I chose the name _impl.rs because of that and the file before I moved it was fn_ctxt_impl.rs.
I am fine with giving it a better name; I just cannot make one with the single brain I have.

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Oct 13, 2020

📌 Commit ce7c73c has been approved by matthewjasper

@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 Oct 13, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#77765 (Add LLVM flags to limit DWARF version to 2 on BSD)
 - rust-lang#77788 (BTreeMap: fix gdb provider on BTreeMap with ZST keys or values)
 - rust-lang#77795 (Codegen backend interface refactor)
 - rust-lang#77808 (Moved the main `impl` for FnCtxt to its own file.)
 - rust-lang#77817 (Switch rustdoc from `clean::Stability` to `rustc_attr::Stability`)
 - rust-lang#77829 (bootstrap: only use compiler-builtins-c if they exist)
 - rust-lang#77870 (Use intra-doc links for links to module-level docs)
 - rust-lang#77897 (Move `Strip` into a separate rustdoc pass)

Failed merges:

 - rust-lang#77879 (Provide better documentation and help messages for x.py setup)
 - rust-lang#77902 (Include aarch64-pc-windows-msvc in the dist manifests)

r? `@ghost`
@bors bors merged commit becd6c6 into rust-lang:master Oct 14, 2020
@rustbot rustbot added this to the 1.49.0 milestone Oct 14, 2020
@Nicholas-Baron Nicholas-Baron deleted the fn_ctxt_impl branch October 14, 2020 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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
Development

Successfully merging this pull request may close these issues.

Splitting up rustc_typeck::check::fn_ctxt::FnCtxt
8 participants