-
Notifications
You must be signed in to change notification settings - Fork 204
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: support for conditional stores #2553
Conversation
This looks to be failing with the same bug I have here (#2446). I'm working on a fix as I think I may know the issue. |
The failing test should pass now if you pull master. |
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.
Could we add a test like below to array_dynamic
where idx is greater than the length of x
?
fn regression_mem_op_predicate(mut x: [u32; 5], idx: Field) {
// We should not hit out of bounds here as we have a predicate
// that should not be hit
if idx as u32 < 3 {
x[idx] = 10;
}
assert(x[4] == 111);
}
crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr
Outdated
Show resolved
Hide resolved
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.
Just some comments suggestions mainly, but none are blocking for getting this out
LGTM. We can merge once merge conflicts are fixed |
const/non const index-predicate yes/no-get/set
Description
Problem*
Resolves #2493
Summary*
Array Set with side-effect context are replaced by predicate*value+(1-predicate)array read
The index of array operation is also replaced by predicateindex.
This strategy does not work with empty slice since we would access index 0 and that would still fail with an empty slice, but I believe this case is degenerate enough to not support it.
Documentation
This PR requires documentation updates when merged.
Additional Context
The PR requires ACVM PR 524 for the CI tests to pass.
PR Checklist*
cargo fmt
on default settings.