-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Do not hash allocations to name them. #119458
base: master
Are you sure you want to change the base?
Conversation
r? @TaKO8Ki (rustbot has picked a reviewer for you, use r? to override) |
yeah, i agree that we should just let llvm deal with the unnamed consts lol r? compiler-errors @bors r+ rollup=never |
I added hashing of them to make diffing LLVM IR easier. |
…-errors Do not hash allocations to name them. Computing the stable hash behind an `Allocation` can be quite slow. The given name does not provide a lot of benefit: a hash is not meaningfully easier to understand than an arbitrary index.
@bors treeclosed=100 |
💔 Test failed - checks-actions |
@bors retry Apple runner billing issue. |
This comment has been minimized.
This comment has been minimized.
You are also removing the |
@bjorn3: isn't that just because this is using the anonymous numbering from llvm? is there a reason to keep |
It shows up in the symbol table, right? Seeing just a symbol named 1234 would be less understandable what it is. Especially when decompiling where it could show up as |
@bjorn3 :
No, it does not. Those constants are "private unnamed_addr" for LLVM, and get no symbol in the binary. The only change is the name that appear in LLVM IR dumps. |
7177a04
to
cb6e396
Compare
@bors r+ |
🌲 The tree is currently closed for pull requests below priority 50. This pull request will be tested once the tree is reopened. |
…-errors Do not hash allocations to name them. Computing the stable hash behind an `Allocation` can be quite slow. The given name does not provide a lot of benefit: a hash is not meaningfully easier to understand than an arbitrary index.
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@rustbot author |
⬆️ That still seems like a valid argument? |
This PR is somehow back in the bors queue. It's not currently approved though. I don't know what's going on but I don't want bors to merge random PRs.^^ Since this is not the only affected PR... EDIT(jieyouxu): bors pls don't see this |
@bors r- Someone probably ran a resync? |
Yeah but that shouldn't screw everything up.^^ |
Right, I'm just pointing out that this is typically how PRs get back into the queue. I don't think anyone is gonna fix homu to stop doing that, though. |
Okay it seems it's just "PRs that were approved and then failed CI" that got re-queued. Not catastrophic then, they'll probably fail again.
EDIT(jieyouxu): bors pls don't see this |
☔ The latest upstream changes (presumably #123936) made this pull request unmergeable. Please resolve the merge conflicts. |
Computing the stable hash behind an
Allocation
can be quite slow. The given name does not provide a lot of benefit: a hash is not meaningfully easier to understand than an arbitrary index.