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] introduce a region unification table and use it in dropck #30242

Merged
merged 1 commit into from
Dec 12, 2015

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Dec 6, 2015

Fixes #29844

I would prefer to
(a) make some performance measurements
(b) use the unification table in a few more places
before committing further, but this is probably good enough for beta.

r? @nikomatsakis

@@ -45,6 +45,41 @@ impl<'a, 'tcx> ty::fold::TypeFolder<'tcx> for OpportunisticTypeResolver<'a, 'tcx
}
}

/// The opportunistic type and region resolver is similar to the
/// opportunistic type resolver, but also opportunistly resolves
/// regions. It is useful for canonicalization.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just always resolve regions?

@nikomatsakis
Copy link
Contributor

OK, so, re-reading your comment, I expect that you were probably shooting for a good way to make a kind of minimally invasive (and minimally risky) version of this PR, which makes a lot of sense.

@nikomatsakis
Copy link
Contributor

I like this better than the alternative I proposed, and my main complaints about this PR was that it was not going as far as I would expect -- but that's probably good from the POV of being minimally invasive.
So:

@bors r+

@bors
Copy link
Contributor

bors commented Dec 11, 2015

📌 Commit 80e191f has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors p=1

@bors
Copy link
Contributor

bors commented Dec 12, 2015

⌛ Testing commit 80e191f with merge d824ab1...

@bors
Copy link
Contributor

bors commented Dec 12, 2015

💔 Test failed - auto-mac-64-opt

@alexcrichton
Copy link
Member

@bors: retry

On Fri, Dec 11, 2015 at 11:38 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-opt
http://buildbot.rust-lang.org/builders/auto-mac-64-opt/builds/7361


Reply to this email directly or view it on GitHub
#30242 (comment).

bors added a commit that referenced this pull request Dec 12, 2015
Fixes #29844

I would prefer to
(a) make some performance measurements
(b) use the unification table in a few more places
before committing further, but this is probably good enough for beta.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 12, 2015

⌛ Testing commit 80e191f with merge da31c14...

@bors bors merged commit 80e191f into rust-lang:master Dec 12, 2015
@arielb1
Copy link
Contributor Author

arielb1 commented Dec 12, 2015

@nikomatsakis

This commit causes a 50% regression in typeck performance (not the WIP). I will write a separate patch to fix it.

@dotdash dotdash mentioned this pull request Dec 12, 2015
@pnkfelix
Copy link
Member

I will write a separate patch to fix it.

Namely PR #30368 (right?)

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 14, 2015

Right that was #30368

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants