-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Misc changes related to Miri allocations #50520
Conversation
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☔ The latest upstream changes (presumably #50395) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #50249) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #50710) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on IRC I am very unsure about the last commit. We had a scheme like that when miri was merged, and we kept running into various edge cases with the decoding order. Even creating MCVEs for the panics we were getting was hard, because small changes in the code would change the order of evaluation.
}); | ||
} | ||
} | ||
|
||
impl<'a, 'gcx, M: HashStable<StableHashingContext<'a>>> HashStable<StableHashingContext<'a>> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation could be replaced by a impl_stable_hash_for!
macro call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we have a macro which works for enums with lifetimes.
I've split the last commit into a separate PR. |
src/bootstrap/lib.rs
Outdated
@@ -592,12 +592,20 @@ impl Build { | |||
Path::new(llvm_bindir.trim()).join(exe("FileCheck", &*target)) | |||
} else { | |||
let base = self.llvm_out(self.config.build).join("build"); | |||
let exe = exe("FileCheck", &*target); | |||
if !self.config.ninja && self.config.build.contains("msvc") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put this in a separate PR, I have no idea what this does or why it's here
@bors r+ |
📌 Commit ddc5418 has been approved by |
☀️ Test successful - status-appveyor, status-travis |
FWIW this hurt the max-rss usage on a few benchmarks (but not by huge amounts) |
Might be due to the removed cache for strings and byte strings |
This builds on top of #50249
r? @oli-obk