circuit.rs: Introduce AssignedCell struct.
#383
Conversation
Codecov Report
@@ Coverage Diff @@
## main #383 +/- ##
==========================================
- Coverage 70.65% 70.38% -0.27%
==========================================
Files 42 42
Lines 5227 5278 +51
==========================================
+ Hits 3693 3715 +22
- Misses 1534 1563 +29
Continue to review full report at Codecov.
|
therealyingtong
force-pushed
the
allocated-cell
branch
from
46d8c7c to
9573efb
Compare
therealyingtong
changed the title
AllocatedCell struct.AssignedCell struct.
therealyingtong
force-pushed
the
allocated-cell
branch
from
f05cd1c to
3745ace
Compare
therealyingtong
force-pushed
the
allocated-cell
branch
from
6dd4c63 to
ed94e18
Compare
therealyingtong
force-pushed
the
allocated-cell
branch
from
ed94e18 to
61c431e
Compare
We only need V: Clone and for<'v> Assigned<F>: From<&'v V> for specific methods. Co-authored-by: Jack Grigg <jack@electriccoin.co> Co-authored-by: Daira Hopwood <daira@jacaranda.org>
therealyingtong
force-pushed
the
allocated-cell
branch
from
4bd5bae to
839f9ec
Compare
| @@ -110,7 +110,7 @@ fn bench_with_k(name: &str, k: u32, c: &mut Criterion) { | |||
| region.assign_fixed(|| "b", self.config.sb, 0, || Ok(FF::zero()))?; | |||
| region.assign_fixed(|| "c", self.config.sc, 0, || Ok(FF::one()))?; | |||
| region.assign_fixed(|| "a * b", self.config.sm, 0, || Ok(FF::one()))?; | |||
| Ok((lhs, rhs, out)) | |||
| Ok((lhs.cell(), rhs.cell(), out.cell())) | |||
There was a problem hiding this comment.
We should clean up the examples to use the newer APIs where possible. I've opened #412 to track this.
| @@ -92,6 +92,64 @@ pub struct Cell { | |||
| column: Column<Any>, | |||
| } | |||
|
|
|||
| /// An assigned cell. | |||
| #[derive(Clone, Debug)] | |||
There was a problem hiding this comment.
Something neat I probably already knew but just became aware of is that if you add #[derive(Clone)] to a struct with generics, Rust will implement it in a way that the struct is Clone when its type parameters are all Clone.
I think it's okay to have impl Clone for AssignedCell, because the underlying Cell is a pointer to the actual cell, so what we'd be cloning is the pointer in addition to the value, which is fine to do without layouter awareness. I don't believe this will be confused with AssignedCell::copy_advice, which clones only the value and assigns a new Cell, because Clone can't be used to satisfy a constraint (as the cell will be in the wrong location).
| let mut value = None; | ||
| let cell = | ||
| self.region | ||
| .assign_advice(&|| annotation().into(), column, offset, &mut || { |
There was a problem hiding this comment.
Why isn't the annotation just passed in directly in these sorts of places, instead of wrapping it in another closure?
There was a problem hiding this comment.
Lifetimes IIRC. This pattern is unaltered by this PR.
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
str4d
force-pushed
the
allocated-cell
branch
from
c556c92 to
4b8f0f0
Compare
|
GitHub wouldn't let me batch all of @daira's code comment suggestions, so I created several commits, then no-op force-pushed to squash them into a single commit. |
| b.0.copy_advice(|| "rhs", &mut region, config.advice[1], 0)?; | ||
|
|
||
| // Now we can assign the multiplication result, which is to be assigned | ||
| // into the output position. |
There was a problem hiding this comment.
This comment was missed; change it to be like the others ("Now we can compute the multiplication result, which is to be assigned into the output position.").
There was a problem hiding this comment.
To avoid another round of re-ACKs, I'll defer this to #412.
In #383 we altered the bounds on region assignment methods like `Region::assign_advice` to constrain the value closure's result on `for<'vr> Assigned<F>: From<&'vr VR>` instead of `VR: Into<Assigned<F>>`. This had the unintended side-effect that `Assigned<F>` could no longer be returned from the closure, because we were previously relying on the implicit `impl From<T> for T` provided by Rust, which no longer fits the bound. This commit adds the missing from-reference impl to restore functionality, re-enabling inversion deferrment.
In #383 we altered the bounds on region assignment methods like `Region::assign_advice` to constrain the value closure's result on `for<'vr> Assigned<F>: From<&'vr VR>` instead of `VR: Into<Assigned<F>>`. This had the unintended side-effect that `Assigned<F>` could no longer be returned from the closure, because we were previously relying on the implicit `impl From<T> for T` provided by Rust, which no longer fits the bound. This commit adds the missing from-reference impl to restore functionality, re-enabling inversion deferrment.
In #383 we altered the bounds on region assignment methods like `Region::assign_advice` to constrain the value closure's result on `for<'vr> Assigned<F>: From<&'vr VR>` instead of `VR: Into<Assigned<F>>`. This had the unintended side-effect that `Assigned<F>` could no longer be returned from the closure, because we were previously relying on the implicit `impl From<T> for T` provided by Rust, which no longer fits the bound. This commit adds the missing from-reference impl to restore functionality, re-enabling inversion deferrment.
Addresses #288.