-
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
stop considering location when computing outlives relationships #50593
Conversation
Instead of tracking the "cause" of each bit that gets added, try to recover that by walking outlives relationships. This is currently imprecise, since it ignores the "point" where the outlives relationship is incurred -- but that's ok, since we're about to stop considering that overall in a later commit. This does seem to affect one error message negatively, I didn't dig *too* hard to find out why.
I confess I've not fully run the tests locally btw, and certainly not since rebasing, so we'll see what Travis thinks. =) But at least |
@bors try |
stop considering location when computing outlives relationships This doesn't (yet?) use SEME regions, but it does ignore the location for outlives constraints. This makes (I believe) NLL significantly faster -- but we should do some benchmarks. It regresses the "get-default" family of use cases for NLL, which is a shame, but keeps the other benefits, and thus represents a decent step forward. r? @pnkfelix
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 |
☀️ Test successful - status-travis |
@Mark-Simulacrum you gonna do a perf run? |
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 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 |
It no longer passes, and the ui test subsumes it anyhow.
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 |
We no longer get two disjoint uses. =)
Ping from triage @pnkfelix! This PR needs your review. |
i'll admit I was waiting for the perf run, but I'll take a look |
@@ -1095,7 +1097,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { | |||
|
|||
/// Tries to finds a good span to blame for the fact that `fr1` |
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.
might be nice to also fix the comment here to reflect the more general behavior
@@ -1112,7 +1097,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { | |||
let relevant_constraint = self.constraints | |||
.iter_enumerated() | |||
.filter_map(|(i, constraint)| { | |||
if self.liveness_constraints.contains(constraint.sub, elem) { | |||
if !self.liveness_constraints.contains(constraint.sub, elem) { |
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.
(it would be nice if this one-line bugfix were attached to the commit that generalized blame_constraint
)
This seems fine to me. I had very small nits. I don't want to r+ because I suspect @nikomatsakis may want to rebase it a little first? But if he's happy with the commit series the way it is, then go ahead and r=me it. |
@bors r+ |
📌 Commit a64ef13 has been approved by |
Normally I would clean up but I'm too busy today |
@bors p=10 |
stop considering location when computing outlives relationships This doesn't (yet?) use SEME regions, but it does ignore the location for outlives constraints. This makes (I believe) NLL significantly faster -- but we should do some benchmarks. It regresses the "get-default" family of use cases for NLL, which is a shame, but keeps the other benefits, and thus represents a decent step forward. r? @pnkfelix
☀️ Test successful - status-appveyor, status-travis |
This doesn't (yet?) use SEME regions, but it does ignore the location for outlives constraints. This makes (I believe) NLL significantly faster -- but we should do some benchmarks. It regresses the "get-default" family of use cases for NLL, which is a shame, but keeps the other benefits, and thus represents a decent step forward.
r? @pnkfelix