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

Fix wrong condition in base::internalize_symbols(). #34917

Merged
merged 3 commits into from
Jul 22, 2016

Conversation

michaelwoerister
Copy link
Member

Fix a typo that snuck into #34899 (and completely broke internalize_symbols()).

@rust-highfive
Copy link
Collaborator

r? @Aatch

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb
Copy link
Member

eddyb commented Jul 19, 2016

Would be nice if you could add a test, but otherwise r=me.

@TimNN
Copy link
Contributor

TimNN commented Jul 19, 2016

This fixes #34891 for me.

@michaelwoerister
Copy link
Member Author

@TimNN Excellent :)

@eddyb
Copy link
Member

eddyb commented Jul 19, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit f782088 has been approved by eddyb

@TimNN
Copy link
Contributor

TimNN commented Jul 19, 2016

The travis failure looks legit to me.

Edit: Making static BAZ: i32 = 21; pub (as the warning suggests) fixes the test failure for me.

@eddyb
Copy link
Member

eddyb commented Jul 19, 2016

@bors r- 'till that's fixed.

@michaelwoerister
Copy link
Member Author

Yes, the error looks legit. The internalizer does not take the #[no_mangle] and explicit linkage attributes into account, something that just never was tested before because it only ran with multiple codegen units enabled. If have a fix for this but I want to test it some more and maybe write a regression test too.

@michaelwoerister
Copy link
Member Author

Alright, that seems to have worked. @eddyb, care to review that last commit too?


// Collect all external declarations in all compilation units.
for ccx in cx.iter() {
// Collect all symbols that need stay externally visible because they
Copy link
Member Author

Choose a reason for hiding this comment

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

whoops, typo: "need to stay"

@eddyb
Copy link
Member

eddyb commented Jul 20, 2016

r=me with the typo fixed.

@michaelwoerister michaelwoerister force-pushed the fix-internalize-symbols branch from e5c4b16 to ecc1295 Compare July 20, 2016 14:26
@michaelwoerister
Copy link
Member Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Jul 20, 2016

📌 Commit ecc1295 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Jul 21, 2016

⌛ Testing commit ecc1295 with merge a7af87c...

@bors
Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-linux-64-cargotest

@alexcrichton
Copy link
Member

@bors: retry

On Wed, Jul 20, 2016 at 5:51 PM, bors notifications@github.com wrote:

💔 Test failed - auto-linux-64-cargotest
https://buildbot.rust-lang.org/builders/auto-linux-64-cargotest/builds/1191


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#34917 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95M3Z6eAqOczt11tBxcxc9BMemI6Eks5qXsKCgaJpZM4JPj8Z
.

@bors
Copy link
Contributor

bors commented Jul 21, 2016

⌛ Testing commit ecc1295 with merge 1caeab7...

@bors
Copy link
Contributor

bors commented Jul 21, 2016

💔 Test failed - auto-win-gnu-32-opt

@alexcrichton
Copy link
Member

@bors: retry

sorry for the number of retries...

On Thu, Jul 21, 2016 at 12:30 PM, bors notifications@github.com wrote:

💔 Test failed - auto-win-gnu-32-opt
https://buildbot.rust-lang.org/builders/auto-win-gnu-32-opt/builds/5020


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#34917 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD95GciAmMEkaYNbx-mnGa30ybPDEBeks5qX8i_gaJpZM4JPj8Z
.

@nikomatsakis
Copy link
Contributor

@bors p=1

this may fix a huge regr in compiler perf

@bors
Copy link
Contributor

bors commented Jul 22, 2016

⌛ Testing commit ecc1295 with merge af87681...

bors added a commit that referenced this pull request Jul 22, 2016
Fix wrong condition in base::internalize_symbols().

Fix a typo that snuck into #34899 (and completely broke `internalize_symbols()`).
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.

8 participants