Create row struct #1510
Create row struct #1510
Conversation
chriseth
force-pushed
the
create_row_struct
branch
from
955c60f
to
76d9772
Compare
chriseth
force-pushed
the
create_row_struct
branch
from
f5edc92
to
562154c
Compare
chriseth
force-pushed
the
create_row_struct
branch
from
562154c
to
08dc3e3
Compare
| pub struct Row<'a, T: FieldElement> { | ||
| /// The values in the row, zero if unknown. | ||
| values: WitnessColumnMap<CellValue<T>>, | ||
| // TODO remove this. |
| } | ||
| } | ||
|
|
||
| impl<'a, T: FieldElement> Row<'a, T> { | ||
| /// Creates a "fresh" row, i.e., one that is empty but initialized with the global range constraints. | ||
| pub fn fresh(fixed_data: &'a FixedData<'a, T>, row: RowIndex) -> Row<'a, T> { | ||
| WitnessColumnMap::from( | ||
| // TODO this instance could be computed exactly once (per column set) and then cloned. |
There was a problem hiding this comment.
Will do these in a subsequent PR
chriseth
force-pushed
the
create_row_struct
branch
from
08dc3e3
to
402c99b
Compare
There was a problem hiding this comment.
Cool!
I think the Default implementation for ColumnMap is broken. In the current code it is only used by take() when the row is replaced anyway, but I think the semantics of Default should be to give you a valid object. Would there be another solution? I guess we could store Option<WitnessColumnMap<T>> in the row, but also kind of annoying.
| enum CellValue<T: FieldElement> { | ||
| Known(T), | ||
| RangeConstraint(RangeConstraint<T>), | ||
| #[default] |
There was a problem hiding this comment.
Nice, didn't know this!
There was a problem hiding this comment.
Me neither, but fortunately, clippy does!
executor/src/witgen/rows.rs
Outdated
| } | ||
| #[derive(Clone, Default, PartialEq)] | ||
| pub struct Row<'a, T: FieldElement> { | ||
| /// The values in the row, zero if unknown. |
There was a problem hiding this comment.
Why "zero if unknown"? It explicitly stores unknown, right?
There was a problem hiding this comment.
True, this was an old comment.
| impl<V, T: PolynomialTypeTrait> Default for ColumnMap<V, T> { | ||
| fn default() -> Self { | ||
| ColumnMap { | ||
| values: Vec::new(), | ||
| _ptype: PhantomData, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hmm, but this would fail, right?
let foo: WitnessColumnMap<u8> = Default::default();
foo[&any_poly_id] = 3;The assumption that values has an entry for every possible key is violated.
executor/src/witgen/rows.rs
Outdated
| }; | ||
| f.write_str(&debug_str) | ||
| self.values = WitnessColumnMap::from( | ||
| std::mem::take(&mut self.values) |
There was a problem hiding this comment.
I think this take() is why you need ColumnMap to implement Default, right?
I just went through the two calls of merge_with and I think it is a small change for it to receive other: Row<'a, T>. So then you could call other.values_into_iter(). So you can trade cloning cell2 for cloning cell1, and don't need a broken Default implementation ^^
|
|
||
| impl<T: FieldElement> Debug for Cell<'_, T> { |
There was a problem hiding this comment.
Hmm, maybe we could have a CellValue::render() function that takes a reference to FixedData? Now this code moved to Row::render_values, but I think it belongs to CellData!
| self.get_row_mut(poly.next).value_is_known(&poly.poly_id) | ||
| } | ||
|
|
||
| fn get_row_mut(&self, next: bool) -> &Row<'a, T> { | ||
| match next { | ||
| false => self.current, | ||
| true => self | ||
| .next | ||
| .unwrap_or_else(|| panic!("Tried to access next row, but it is not available.")), | ||
| } | ||
| } |
There was a problem hiding this comment.
| self.get_row_mut(poly.next).value_is_known(&poly.poly_id) | |
| } | |
| fn get_row_mut(&self, next: bool) -> &Row<'a, T> { | |
| match next { | |
| false => self.current, | |
| true => self | |
| .next | |
| .unwrap_or_else(|| panic!("Tried to access next row, but it is not available.")), | |
| } | |
| } | |
| self.get_row(poly.next).value_is_known(&poly.poly_id) | |
| } |
Right?
No description provided.