Skip to content

/rust

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

incr.comp.: Fix HashStable for ty::RegionKind and trans item order #45176

Merged
merged 2 commits into from Oct 12, 2017

Conversation

Reviewers

@nikomatsakis nikomatsakis

Assignees

@nikomatsakis nikomatsakis

Labels
beta-accepted
Projects
None yet
Milestone
No milestone
5 participants
@michaelwoerister @nikomatsakis @bors @alexcrichton @kennytm
@michaelwoerister
Copy link
Contributor

michaelwoerister commented Oct 10, 2017

Fixes #45161 and the failing rust-icci tests.

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis Oct 10, 2017

@nikomatsakis

nikomatsakis approved these changes Oct 10, 2017
View changes

Copy link
Contributor

nikomatsakis left a comment

Seems fine. I left a question, but I'll r+ in the meantime.
src/librustc_trans/partitioning.rs
// the codegen tests and can even make item order
// unstable.
InstanceDef::Item(def_id) => {
tcx.hir.as_local_node_id(def_id)

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Oct 10, 2017

Contributor

should we sort instead by def_id?

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Oct 11, 2017

Author Contributor

I'm not sure that DefIds also reflect definition order. They probably do. This whole thing is a bit of a hack anyway. We could sort by Span...

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Oct 11, 2017

Contributor

DefIds do not reflect definition order, except in anonymous cases like impls. Otherwise, they would sort by name. Why do we care if these are reflect "definition order" per se?

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Oct 12, 2017

Author Contributor

I makes writing the codegen tests a lot more pleasant if you know in which order things will appear in LLVM IR. You could of course also run the compiler once to find out what the order is but that's another hurdle to writing such tests. Also the tests might break if we changed the details of our symbol mangling (which we've done quite a few times). What we have now is "OK" in comparison, I think.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Oct 10, 2017

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 10, 2017

📌 Commit 1235836 has been approved by nikomatsakis

@michaelwoerister michaelwoerister referenced this pull request Oct 12, 2017

Closed

ICE: /checkout/src/librustc/ich/impls_ty.rs:78: TypeIdHasher: unexpected region ReLateBound(DebruijnIndex { depth: 1 }, "BrEnv") #45161

@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Oct 12, 2017

@bors p=1

Taking the liberty of bumping the priority of this since it will (hopefully) unbreak perf.rlo.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 12, 2017

⌛️ Testing commit 1235836 with merge 1807f27...

bors added a commit that referenced this pull request Oct 12, 2017

@bors
Auto merge of #45176 - michaelwoerister:fix-region-and-trans-item-ord… 
…er, r=nikomatsakis

incr.comp.: Fix HashStable for ty::RegionKind and trans item order

Fixes #45161 and the failing rust-icci tests.

r? @nikomatsakis
Loading status checks…
1807f27
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 12, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 1807f27 to master...

@bors bors merged commit 1235836 into rust-lang:master Oct 12, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member

alexcrichton commented Oct 19, 2017

@michaelwoerister @nikomatsakis the bootstrap compiler update has bounced I believe due to #45161 and I think this PR is not currently in beta. Any objections to backporting this?

@alexcrichton alexcrichton referenced this pull request Oct 19, 2017

Merged

Bump to 1.23 and update bootstrap #45285

@kennytm kennytm added the beta-nominated label Oct 19, 2017

@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Oct 20, 2017

No objections.

@alexcrichton alexcrichton added the beta-accepted label Oct 20, 2017

@alexcrichton alexcrichton referenced this pull request Oct 20, 2017

Merged

[beta] Backport incremental compilation fix #45413

@alexcrichton alexcrichton removed the beta-nominated label Oct 20, 2017

bors added a commit that referenced this pull request Oct 20, 2017

@bors
Auto merge of #45413 - alexcrichton:beta-next, r=Mark-Simulacrum 
[beta] Backport incremental compilation fix

This is a backport of #45176 along with a bump of the beta number so we can hopefully update the bootstrap compiler in #45285
Loading status checks…
a0a837c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.