-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Lifetime cleanups #130294
Lifetime cleanups #130294
Conversation
This does change the external interface, but not in a way that will cause any breakage because external users don't mention the lifetimes.
Giving them more typical names.
- Replace non-standard names like 's, 'p, 'rg, 'ck, 'parent, 'this, and 'me with vanilla 'a. These are cases where the original name isn't really any more informative than 'a. - Replace names like 'cx, 'mir, and 'body with vanilla 'a when the lifetime applies to multiple fields and so the original lifetime name isn't really accurate. - Put 'tcx last in lifetime lists, and 'a before 'b.
Could not assign reviewer from: |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri |
compiler/rustc_passes/src/loops.rs
Outdated
struct CheckLoopVisitor<'a, 'tcx> { | ||
sess: &'a Session, | ||
struct CheckLoopVisitor<'tcx> { | ||
sess: &'tcx Session, |
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.
We can get the session from the tcx, the field can be removed
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've added a commit doing that.
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 after oli's review.
I feel like this change is right at the edge of whether it's worth it given the potential merge conflicts, reviewer time and what not. So while I think it's an improvement and we should merge this PR, I would not like to review many more PRs like this.
The good news is that this PR does most of the these sorts of possible changes :) |
@bors r+ rollup=iffy |
Rollup of 6 pull requests Successful merges: - rust-lang#130017 (coverage: Extract `executor::block_on` from several async coverage tests) - rust-lang#130268 (simd_shuffle: require index argument to be a vector) - rust-lang#130290 (Stabilize entry_insert) - rust-lang#130294 (Lifetime cleanups) - rust-lang#130343 (docs: Enable required feature for 'closure_returning_async_block' lint) - rust-lang#130349 (Fix `Parser::break_up_float`'s right span) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#130294 - nnethercote:more-lifetimes, r=lcnr Lifetime cleanups The last commit is very opinionated, let's see how we go. r? `@oli-obk`
FWIW, how I got to this PR:
|
The last commit is very opinionated, let's see how we go.
r? @oli-obk