trans: Internalize symbols without relying on LLVM#43183
trans: Internalize symbols without relying on LLVM#43183bors merged 6 commits intorust-lang:masterfrom
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
There was a problem hiding this comment.
Does this need to be Clone? It looks like it's a fairly large structure that ideally we wouldn't clone. It seems like it's in an Arc most of the time anyway so this feels unnecessary.
There was a problem hiding this comment.
That is indeed not necessary anymore. Will remove.
src/librustc_trans/collector.rs
Outdated
There was a problem hiding this comment.
It looks like what we really want here is an ExactSizeIterator?
There was a problem hiding this comment.
I'll look into it.
src/librustc_trans/partitioning.rs
Outdated
There was a problem hiding this comment.
This appears to no longer be necessary.
There was a problem hiding this comment.
I don't understand. It's used in the nested continue below.
There was a problem hiding this comment.
I'm probably just missing something, but I don't see a nested loop above it?
There was a problem hiding this comment.
Oh right, the only place those nested loops still exist is in my head :) Sorry for the confusion. I'll remove the label.
src/librustc_trans/partitioning.rs
Outdated
There was a problem hiding this comment.
Why do we need to clone the option? It looks like that shouldn't be necessary -- we only use it for PartialEq?
There was a problem hiding this comment.
cloned() (as opposed to clone()) turns the Option<&T> into an Option<T>, which is what we need for filter_map.
There was a problem hiding this comment.
I see what you meant now. Yes, cloning seems unnecessary.
src/librustc_trans/collector.rs
Outdated
There was a problem hiding this comment.
I recommend tcx.is_const_fn(def_id) - similarly for hir::Generics, ty::Generics should be preferred, although that one is preexisting.
There was a problem hiding this comment.
Does is_const_fn do something differently than the code below? I'm not against changing, but the constness and generics from the HIR item are right there without the need for a hashtable lookup.
src/librustc_trans/collector.rs
Outdated
|
r=me modulo nits and the build failure |
ad74e77 to
2b9ab65
Compare
2b9ab65 to
2f07eb3
Compare
efdc922 to
c93e62b
Compare
|
@bors r=eddyb All nits addressed, I think. |
|
📌 Commit 678d377 has been approved by |
|
⌛ Testing commit 678d377 with merge 685ccdf38707dc948ec9bbde82caa8c9e0ac5cdc... |
|
💔 Test failed - status-travis |
|
|
@bors r=eddyb That assertion should be fixed, hopefully. |
|
📌 Commit 226bc92 has been approved by |
|
⌛ Testing commit 226bc92 with merge 82e2f9ddd4d0d32bb1ab96928e099c49975d932c... |
|
💔 Test failed - status-travis |
|
@bors retry |
|
⌛ Testing commit 226bc92 with merge b45d8f0ebcf760610826ce2a43f5f3d437916086... |
|
💔 Test failed - status-appveyor |
|
Spurious. Could this be related to https://appveyor.statuspage.io/incidents/m2vdvw39kdk8? |
|
@bors retry (No harm in retrying while the queue is empty, I guess) |
|
⌛ Testing commit 226bc92 with merge 49765af5c6bcd1182b8d279b404d79fdd8a81c18... |
|
💔 Test failed - status-appveyor |
|
@bors: retry
…On Wed, Jul 19, 2017 at 6:59 AM, bors ***@***.***> wrote:
💔 Test failed - status-appveyor
<https://ci.appveyor.com/project/rust-lang/rust/build/1.0.3983>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#43183 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AaPN0DaQBIlhKb8psY9iNfX3uCfRj9k0ks5sPe-hgaJpZM4OV07r>
.
--
You received this message because you are subscribed to the Google Groups
"rust-ops" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to ***@***.***
To post to this group, send email to ***@***.***
To view this discussion on the web visit https://groups.google.com/d/
msgid/rust-ops/rust-lang/rust/pull/43183/c316363043%40github.com
<https://groups.google.com/d/msgid/rust-ops/rust-lang/rust/pull/43183/c316363043%40github.com?utm_medium=email&utm_source=footer>
.
For more options, visit https://groups.google.com/d/optout.
|
|
@nagbot-rs: 🔑 Insufficient privileges: and not in try users |
|
@bors: retry |
|
⌛ Testing commit 226bc92 with merge 528330520a5ad6953e4a0f25f79d4db1ad019e7f... |
|
💔 Test failed - status-appveyor |
|
|
|
Thanks, @kennytm! |
(and a few more) |
42c4564 to
e8ff75b
Compare
e8ff75b to
f6e5416
Compare
|
📌 Commit f6e5416 has been approved by |
…lvm, r=eddyb trans: Internalize symbols without relying on LLVM This PR makes the compiler use the information gather by the trans collector in order to determine which symbols/trans-items can be made internal. This has the advantages: + of being LLVM independent, + of also working in incremental mode, and + of allowing to not keep all LLVM modules in memory at the same time. This is in preparation for fixing issue #39280. cc @rust-lang/compiler
|
☀️ Test successful - status-appveyor, status-travis |
This PR makes the compiler use the information gather by the trans collector in order to determine which symbols/trans-items can be made internal. This has the advantages:
This is in preparation for fixing issue #39280.
cc @rust-lang/compiler