Conversation
|
It seems to me that using a no-op for |
|
Thanks @zhassan-aws for the detailed explanation! To be clear, I was under the impression this was not sound because I thought we had to generate code for "forgetting" the value in CBMC. But as you explained we do not need to do anything during code generation, so a no-op would be fine. Now, instead of removing it, I was thinking of adding two tests similar to the example found here but using this fn main() {
let mut v = vec![65, 122];
let s = unsafe { String::from_raw_parts(v.as_mut_ptr(), v.len(), v.capacity()) };
std::intrinsics::forget(v);
assert!(v[0] == 65); // this causes failure to codegen
assert_eq!(s, "Az");
}This should fail to codegen, and another test not including the first assertion should pass. What do you think? |
|
Just pushed the changes I was proposing because I figured it's something we would like to have anyway. Updated title and description as well. |
zhassan-aws
left a comment
There was a problem hiding this comment.
Makes sense to me (technically it would be a compilation error as opposed to a codegen failure with the first assert, but I guess for the purpose of compiletest, it's the same thing).
|
Right, switched to |
* Disable `forget` intrinsic * Restore `forget` * Add two tests for `forget` * Update `forget` status in support table * Use `check-fail` instead of `codegen-fail`
Description of changes:
Adds a positive and a negative test case for
forgetand updates the support table accordingly.Resolved issues:
Part of #727
Call-outs:
PR was originally "Disable
forgetintrinsic" but was changed after discussion below. Description included below for reference.Testing:
How is this change tested? Existing regression.
Is this a refactor change? No.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.