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

Expand the "givens" set to cover transitive relations. #24527

Merged
merged 1 commit into from
Jun 20, 2015

Conversation

nikomatsakis
Copy link
Contributor

Expand the "givens" set to cover transitive relations. The givens array
stores relationships like 'c <= '0 (where 'c is a free region and
'0 is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like 'c <= '0 and '0 <= '1. Fixes #24085.

r? @pnkfelix
cc @bkoropoff

But I am not sure whether this fix will have a compile-time hit. I'd like to push to try branch observe cycle times.

@nikomatsakis
Copy link
Contributor Author

@bors try 11720a1

@bors
Copy link
Contributor

bors commented Apr 17, 2015

⌛ Trying commit 11720a1 with merge d636d5e...

bors added a commit that referenced this pull request Apr 17, 2015
Expand the "givens" set to cover transitive relations.  The givens array
stores relationships like `'c <= '0` (where `'c` is a free region and
`'0` is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like `'c <= '0` and `'0 <= '1`. Fixes #24085.

r? @pnkfelix 
cc @bkoropoff

*But* I am not sure whether this fix will have a compile-time hit. I'd like to push to try branch observe cycle times.
@bors
Copy link
Contributor

bors commented Apr 17, 2015

💔 Test failed - try-bsd

@pnkfelix
Copy link
Member

@nikomatsakis what timing info were you hoping to extract from the try run?

@bors
Copy link
Contributor

bors commented Apr 18, 2015

☔ The latest upstream changes (presumably #24209) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor Author

@pnkfelix I guess just an overall cycle time effect.

@steveklabnik
Copy link
Member

Is this PR still valid/needed/wanted?

@bkoropoff
Copy link
Contributor

Although I worked around the bug, I would like it to be fixed.

On Fri, Jun 12, 2015 at 2:41 PM Steve Klabnik notifications@github.com
wrote:

Is this PR still valid/needed/wanted?


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

stores relationships like `'c <= '0` (where `'c` is a free region and
`'0` is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like `'c <= '0` and `'0 <= '1`. Fixes rust-lang#24085.
@nikomatsakis
Copy link
Contributor Author

OK, I rebased this branch, and compared it's effect on compilaton time. It seems to be negligible to non-existent, at least when building rustc.

@nikomatsakis
Copy link
Contributor Author

@bors r+

@bors
Copy link
Contributor

bors commented Jun 19, 2015

📌 Commit 29c8653 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor Author

For reference, using this branch, compiling librustc and all descendants spent 34.962s in type-checking. On master it took 35.013s.

bors added a commit that referenced this pull request Jun 19, 2015
Expand the "givens" set to cover transitive relations.  The givens array
stores relationships like `'c <= '0` (where `'c` is a free region and
`'0` is an inference variable) that are derived from closure
arguments. These are (rather hackily) ignored for purposes of inference,
preventing spurious errors. The current code did not handle transitive
cases like `'c <= '0` and `'0 <= '1`. Fixes #24085.

r? @pnkfelix 
cc @bkoropoff

*But* I am not sure whether this fix will have a compile-time hit. I'd like to push to try branch observe cycle times.
@bors
Copy link
Contributor

bors commented Jun 19, 2015

⌛ Testing commit 29c8653 with merge 4b42cbd...

@bors bors merged commit 29c8653 into rust-lang:master Jun 20, 2015
@nikomatsakis nikomatsakis deleted the issue-24085 branch March 30, 2016 16:14
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.

Adding Copy impl to type causes lifetime errors
6 participants