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

[WIP] Don't re-export rustc_ast items in rustc_hir #77494

Closed
wants to merge 1 commit into from

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Oct 3, 2020

  • BorrowKind
  • ImplPolarity
  • IsAuto
  • CaptureBy
  • Movability
  • Mutability

While working on this I realized that there are many different enums
named BorrowKind, so it should really keep the prefix and just change
it from hir:: to ast::.

Partial fix for rust-lang/compiler-team#368.

r? @ghost - I still need to fix all the warnings and move BorrowKind back to ast::BorrowKind

@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. A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate labels Oct 3, 2020
- BorrowKind
- ImplPolarity
- IsAuto
- CaptureBy
- Movability
- Mutability

While working on this I realized that there are many different enums
named `BorrowKind`, so it should really keep the prefix and just change
it from `hir::` to `ast::`.
@jyn514
Copy link
Member Author

jyn514 commented Oct 3, 2020

@bors rollup=never

This will cause a ton of conflicts.

@jyn514 jyn514 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 3, 2020
@jyn514 jyn514 changed the title [WIP] Don't re-export rustc_span items in rustc_hir [WIP] Don't re-export rustc_ast items in rustc_hir Oct 3, 2020
@petrochenkov petrochenkov self-assigned this Oct 3, 2020
@ecstatic-morse
Copy link
Contributor

Please don't do this. Now every time I work with the HIR, I have to remember that Mutability is actually defined in rustc_ast? What are we gaining here? How does writing hir::ExprKind::AddrOf(ast::BorrowKind::Ref, ast::Mutability::Mut, e) make for better code?

@jyn514
Copy link
Member Author

jyn514 commented Oct 3, 2020

The goal is to reduce interdependence in the rustc_* crates by making it clear where things come from; hopefully this will allow us to remove some crates from Cargo.toml, making the build more parallel. I also think this will make it easier to split up rustc_middle (#65031).

Maybe rustc_hir was a bad place to start? Would you be ok with instead removing the pub use rustc_hir::Mutability in rustc_middle::mir?

@ecstatic-morse
Copy link
Contributor

I don't care if my 20 minute compilation gets a few seconds faster if it becomes more difficult to write code.

hopefully this will allow us to remove some crates from Cargo.toml, making the build more parallel. I also think this will make it easier to split up rustc_middle.

I don't buy the first part. Luckily we have benchmarks for rustc on the perf server now, so there's no need to hope. These changes either make things faster or they don't. Compile times for rustc_middle are dominated by the query system. If you want to make the build meaningfully faster, look there.

@petrochenkov
Copy link
Contributor

hopefully this will allow us to remove some crates from Cargo.toml, making the build more parallel

That is unlikely to happen with rustc_ast and rustc_hir though.
Also, if some pieces of AST are lowered to HIR trivially, then reexporting them instead of introducing new types will avoid extra work and extra code.

@jyn514
Copy link
Member Author

jyn514 commented Oct 3, 2020

Ok, this change is not super useful then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cross-crate-reexports Area: Documentation that has been re-exported from a different crate C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

3 participants