Region naming refactoring [6/N] #67476
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
rustbot
added
S-blocked
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Centril
reviewed
Dec 21, 2019
src/librustc_mir/borrow_check/diagnostics/outlives_suggestion.rs
Outdated
Show resolved
Hide resolved
Dylan-DPC-zz
removed
the
S-waiting-on-author
mark-i-m
force-pushed
the
simplify-borrow_check-5
branch
from
c49dab5
to
1a58035
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
mark-i-m
force-pushed
the
simplify-borrow_check-5
branch
from
1a58035
to
d7e6fc3
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts. |
mark-i-m
force-pushed
the
simplify-borrow_check-5
branch
from
36151eb
to
d09d949
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
mark-i-m
force-pushed
the
simplify-borrow_check-5
branch
from
d09d949
to
8161024
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@matthewjasper @eddyb Ok, this looks ready for review, though it is still waiting for the previous PR to merge. The major changes in this PR include:
The PR is rather large, but the commits are fairly self-contained (though they don't all compile). There was one minor output change to one test with Between this PR and the last one, a net of 200 lines are removed, most of which was function parameters and context structs 🎉 EDIT: also copied this ^^ to the OP... |
|
Oh, forgot to mention: I realize this is a big PR, and I can think of at least 3 smaller PRs that this could be split into if you would like (though I would prefer to get it over with :P) |
|
☔ The latest upstream changes (presumably #66942) made this pull request unmergeable. Please resolve the merge conflicts. |
mark-i-m
force-pushed
the
simplify-borrow_check-5
branch
2 times, most recently
from
f8ae596
to
b5a6656
Compare
|
@rustbot modify labels: +S-waiting-on-review -S-blocked |
rustbot
removed
the
S-blocked
matthewjasper
added
S-waiting-on-author
mark-i-m
force-pushed
the
simplify-borrow_check-5
branch
from
bebda54
to
f05e40e
Compare
|
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author |
rustbot
added
S-waiting-on-review
|
The diffs in the PR description make me very confused as to why this wasn't done from the start. |
|
Is bors not looking at reviews? @bors r+ |
|
📌 Commit f05e40e has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jan 17, 2020
Region naming refactoring [6/N] Followup to #67474 EDIT: this PR is probably best read commit-by-commit... The major changes in this PR include: - moving many functions around to modules that better suit them. In particular, a lot of methods were moved from `borrow_check::diagnostics::region_errors` to `borrow_check::region_infer`, and `report_region_errors` was moved from `borrow_check` to `borrow_check::diagnostics::region_errors`. - `borrow_check::diagnostics::{region_errors, region_name}` are now most comprised of methods on `MirBorrowckCtxt` instead of `RegionInferenceContext`, allowing us to get rid of the annoying `pub(in crate::borrow_check)` on most of the fields of the latter, along with a number of method arguments on many methods. - I renamed `MirBorrowckCtxt.nonlexical_regioncx` to just `regioncx` because their is no lexical lifetimes any more, and the old name was annoyingly verbose, causing many lines to wrap unnecessarily. - I got rid of `ErrorRegionNamingContext`. Region naming is implemented as inherent methods on `MirBorrowckCtxt`, so we just move the naming stuff into that struct. The PR is rather large, but the commits are fairly self-contained (though they don't all compile). There was one minor output change to one test with `compare-mode=nll`, which I think is acceptable. Between this PR and the last one, a net of 200 lines are removed, most of which was function parameters and context structs :tada: Some samples: ```diff - self.nonlexical_regioncx.free_region_constraint_info( - &self.body, - &self.local_names, - &self.upvars, - self.mir_def_id, - self.infcx, - borrow_region_vid, - region, - ); + self.free_region_constraint_info(borrow_region_vid, region); ``` ```diff - .or_else(|| { - self.give_name_if_anonymous_region_appears_in_yield_ty( - infcx, - body, - *mir_def_id, - fr, - renctx, - ) - }); + .or_else(|| self.give_name_if_anonymous_region_appears_in_arguments(fr)) ``` r? @matthewjasper cc @eddyb
|
I didn't read too closely, but sounds great! ❤️ |
|
☀️ Test successful - checks-azure |
This was referenced Jan 18, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Followup to #67474
EDIT: this PR is probably best read commit-by-commit...
The major changes in this PR include:
borrow_check::diagnostics::region_errorstoborrow_check::region_infer, andreport_region_errorswas moved fromborrow_checktoborrow_check::diagnostics::region_errors.borrow_check::diagnostics::{region_errors, region_name}are now most comprised of methods onMirBorrowckCtxtinstead ofRegionInferenceContext, allowing us to get rid of the annoyingpub(in crate::borrow_check)on most of the fields of the latter, along with a number of method arguments on many methods.MirBorrowckCtxt.nonlexical_regioncxto justregioncxbecause their is no lexical lifetimes any more, and the old name was annoyingly verbose, causing many lines to wrap unnecessarily.ErrorRegionNamingContext. Region naming is implemented as inherent methods onMirBorrowckCtxt, so we just move the naming stuff into that struct.The PR is rather large, but the commits are fairly self-contained (though they don't all compile). There was one minor output change to one test with
compare-mode=nll, which I think is acceptable.Between this PR and the last one, a net of 200 lines are removed, most of which was function parameters and context structs 🎉
Some samples:
r? @matthewjasper
cc @eddyb