incr.comp.: Fix HashStable for ty::RegionKind and trans item order #45176
Conversation
| // the codegen tests and can even make item order | ||
| // unstable. | ||
| InstanceDef::Item(def_id) => { | ||
| tcx.hir.as_local_node_id(def_id) |
There was a problem hiding this comment.
should we sort instead by def_id?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
@bors r+ |
|
📌 Commit 1235836 has been approved by |
|
@bors p=1 Taking the liberty of bumping the priority of this since it will (hopefully) unbreak perf.rlo. |
…er, r=nikomatsakis incr.comp.: Fix HashStable for ty::RegionKind and trans item order Fixes #45161 and the failing rust-icci tests. r? @nikomatsakis
|
☀️ Test successful - status-appveyor, status-travis |
|
@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? |
kennytm
added
the
beta-nominated
|
No objections. |
alexcrichton
added
the
beta-accepted
alexcrichton
removed
the
beta-nominated
Fixes #45161 and the failing rust-icci tests.
r? @nikomatsakis