-
Notifications
You must be signed in to change notification settings - Fork 14
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
(WIP) Prototype for clone-free Expression
#454
Conversation
/// Constant poly | ||
Constant(E::BaseField), | ||
/// This is the sum of two Expr | ||
Sum(CompoundExpr<'a, E>, CompoundExpr<'a, E>), |
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.
No Box
es here!
/// Contains a reference to [`ExprTree`] that is managed by [`ExprBuilder`]. | ||
#[derive(Clone, Copy, Debug)] | ||
pub enum Expr<'a, E: ExtensionField> { | ||
Constant(E::BaseField), |
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.
The enum is for convenience, so you don't need a 'builder' for constants. The whole thing would also work without.
} | ||
|
||
#[derive(Clone, Copy, Debug)] | ||
pub struct CompoundExpr<'a, E: ExtensionField>(&'a ExprTree<'a, E>); |
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.
We need this extra wrapper here to keep Rust's type system happy. For some reason you can't just write Sum(&'a ExprTree<'a, E>, &'a ExprTree<'a, E>),
in the definition of ExprTree
and have it work.
(This wrapper shouldn't have any runtime impact.)
Closing this as obsolete for now. Might resurrect, if there's demand. |
This should help with readability. We want to cut down on visual noise, so that it'll be easier for us and any auditor (and enthusiasts, once this is open sourced) to spot problems with the constraints. Or otherwise convince themselves that there are no problem with the constraints.
In theory, the new approach can be ever so slightly faster, but I don't think it makes much of a difference performance-wise. The technique is borrowed from what the Rust compiler uses internally.