-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: master
Are you sure you want to change the base?
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.)
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.