-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Remove Engine
#132338
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Remove Engine
#132338
Conversation
This is a standard pattern: ``` MyAnalysis.into_engine(tcx, body).iterate_to_fixpoint() ``` `into_engine` and `iterate_to_fixpoint` are always called in pairs, but sometimes with a builder-style `pass_name` call between them. But a builder-style interface is overkill here. This has been bugging me a for a while. This commit: - Merges `Engine::new` and `Engine::iterate_to_fixpoint`. This removes the need for `Engine` to have fields, leaving it as a trivial type that the next commit will remove. - Renames `Analysis::into_engine` as `Analysis::iterate_to_fixpoint`, gives it an extra argument for the optional pass name, and makes it call `Engine::iterate_to_fixpoint` instead of `Engine::new`. This turns the pattern from above into this: ``` MyAnalysis.iterate_to_fixpoint(tcx, body, None) ``` which is shorter at every call site, and there's less plumbing required to support it.
It's no longer needed. `Engine::iterate_to_fixpoint` can be inlined into `Analysis::iterate_to_fixpoint` and removed. The commit also renames `engine.rs` as `results.rs`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
R=me with one nit
.into_engine(tcx, body) | ||
.pass_name("borrowck") | ||
.iterate_to_fixpoint() | ||
.iterate_to_fixpoint(tcx, body, Some("borrowck")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further simplification: should we always give a name and get rid of the option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just took a look. The optional pass name is unwrapped as "-----"
here:
create_dump_file(tcx, "dot", false, A::NAME, &pass_name.unwrap_or("-----"), body)? |
So everywhere I've used None
would be changed to "-----"
which would be weird. More generally, the file naming scheme for IR dumping is complicated. I don't want to delve into that for this PR, which is a straightforward refactoring. Maybe in a follow-up.
Thanks for the fast review. @bors r+ rollup |
Remove `Engine` It's just unnecessary plumbing. Removing it results in less code, and simpler code. r? `@cjgillot`
…kingjubilee Rollup of 8 pull requests Successful merges: - rust-lang#129394 (Don't lint `irrefutable_let_patterns` on leading patterns if `else if` let-chains) - rust-lang#131856 (TypingMode: merge intercrate, reveal, and defining_opaque_types) - rust-lang#132246 (Rename `rustc_abi::Abi` to `BackendRepr`) - rust-lang#132322 (powerpc64-ibm-aix: update maintainters) - rust-lang#132327 (Point to Fuchsia team in platform support docs) - rust-lang#132332 (Use `token_descr` more in error messages) - rust-lang#132338 (Remove `Engine`) - rust-lang#132340 (cg_llvm: Consistently use safe wrapper function `set_section`) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#129394 (Don't lint `irrefutable_let_patterns` on leading patterns if `else if` let-chains) - rust-lang#131856 (TypingMode: merge intercrate, reveal, and defining_opaque_types) - rust-lang#132322 (powerpc64-ibm-aix: update maintainters) - rust-lang#132327 (Point to Fuchsia team in platform support docs) - rust-lang#132332 (Use `token_descr` more in error messages) - rust-lang#132338 (Remove `Engine`) - rust-lang#132340 (cg_llvm: Consistently use safe wrapper function `set_section`) - rust-lang#132342 (cg_llvm: Clean up FFI calls for operand bundles) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#132338 - nnethercote:rm-Engine, r=nnethercote Remove `Engine` It's just unnecessary plumbing. Removing it results in less code, and simpler code. r? ``@cjgillot``
Culprit upstream change: rust-lang/rust#132338 Resolves #3664 By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
Remove `Engine` It's just unnecessary plumbing. Removing it results in less code, and simpler code. r? ``@cjgillot``
It's just unnecessary plumbing. Removing it results in less code, and simpler code.
r? @cjgillot