-
Notifications
You must be signed in to change notification settings - Fork 233
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
chore: Remove Value::Array
in favor of Instruction::MakeArray
#2494
Conversation
Co-authored-by: Maxim Vezenov <[email protected]>
Closing this PR, I don't think it's a large code improvement |
Reopening this, after some second thought I think this may still be valuable. |
b2b10fc
to
76b77f8
Compare
…ing into an empty new block
let expected_opcodes = vec![ | ||
Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(1))), | ||
Opcode::MemoryInit { block_id: BlockId(0), init: vec![Witness(1)] }, | ||
Opcode::Arithmetic(&Expression::one() - &Expression::from(Witness(2))), |
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'm unsure why this memory init is needed now. Did we need it before as well? Neither array is used dynamically.
Still one last bug in
|
Closing this PR, there are too many confilcts to update it. I think the idea of a MakeArray instruction is still a good one and we should revisit this eventually. |
Description
Problem*
Resolves #1733
Summary*
This PR is an experiment to see if removing
Value::Array
simplifies the code at all. Since it is the only Value that may contain other ValueIds, removing it means we no longer have to specially handle and recur on Value::Array when working on ValueIds.So far though, preliminary results of this PR are that there are no large gains made from removing it.
Documentation
This PR requires documentation updates when merged.
Additional Context
PR Checklist*
cargo fmt
on default settings.