-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
const forget tests #69645
const forget tests #69645
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
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 |
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.
@bors r+ rollup
bors doesn't listen to review comments, r+ on behalf of Dylan :) |
📌 Commit a674e1c has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
…lan-DPC const forget tests Adds tests for rust-lang#69617
Rollup of 10 pull requests Successful merges: - #66059 (mem::zeroed/uninit: panic on types that do not permit zero-initialization) - #69373 (Stabilize const for integer {to,from}_{be,le,ne}_bytes methods) - #69591 (Use TypeRelating for instantiating query responses) - #69625 (Implement nth, last, and count for iter::Copied) - #69645 (const forget tests) - #69766 (Make Point `Copy` in arithmetic documentation) - #69825 (make `mem::discriminant` const) - #69859 (fix #62456) - #69891 (Exhaustiveness checking, `Matrix::push`: recursively expand or-patterns) - #69896 (parse: Tweak the function parameter edition check) Failed merges: r? @ghost
|
||
// Call the forget_box at runtime, | ||
// as we can't const-construct a box yet. | ||
const_forget_box(Box::new(0i32)); |
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 test fails in Miri due to a memory leak: it complains that there is a (run-time!) allocation that did not get freed, and that allocation is the Box
being created here.
It almost looks like that is deliberate? Do we really want a leaking test in the test suite? Cc @oli-obk
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 entirely see the point of calling this function at run-time, as it tests nothing about CTFE. Sure, you can't test it at compile-time, but testing at run-time instead makes no sense, it just tests the entirely wrong thing. Am I missing something?
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.
Oh woops, I'm sory, I didn't thought of this failing miri. (Why did the CI still succeed, btw? no miri test
?)
The runtime call was more like a 'Proof' it still works at runtime. It can be removed.
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.
Due to CI budget constraints, running Miri on the libcore test suite is a separate project. That's why CI here did not fail.
It can be removed.
I would prefer that. Can you submit a PR?
Adds tests for #69617