[experiment] Remove PROGRAM_SIZE from type #587
Conversation
ceno_zkvm/src/structs.rs
Outdated
| @@ -152,10 +152,10 @@ impl<E: ExtensionField> ZKVMConstraintSystem<E> { | |||
| config | |||
| } | |||
|
|
|||
| pub fn register_table_circuit<TC: TableCircuit<E>>(&mut self) -> TC::TableConfig { | |||
| pub fn register_table_circuit<TC: TableCircuit<E>>(&mut self, tc: TC) -> TC::TableConfig { | |||
There was a problem hiding this comment.
There could be a convenience method (original name) that uses Default (if implemented) and a new one with the parameter.
impl<TC> …
where TC::TableConfig: Default {
pub fn register_table_circuit(…) {
let tc = Default::default();
…
}
impl<TC> … {
pub fn register_table_circuit_param(…, tc: TC) {
…
}There was a problem hiding this comment.
This way there is no need to change every call.
There was a problem hiding this comment.
👍 agreed, got a bit carried away there
|
I agree that we probably went a bit overboard with trying to using |
| @@ -104,11 +104,22 @@ pub struct ProgramTableConfig { | |||
| mlt: WitIn, | |||
| } | |||
|
|
|||
| pub struct ProgramTableCircuit<E, const PROGRAM_SIZE: usize>(PhantomData<E>); | |||
| #[derive(Clone, Default)] | |||
There was a problem hiding this comment.
Default here is 0 size which is probably not what you want. Just leave it without default to force the use of the …_param function?
| pub trait TableCircuit<E: ExtensionField> { | ||
| pub trait TableCircuit<E: ExtensionField> | ||
| where | ||
| Self: Default, |
There was a problem hiding this comment.
This Default requirement could move as far out as possible towards the function that actually needs it.
| &zkvm_cs, | ||
| &prog_config, | ||
| vm.program(), | ||
| ptc_with_size, |
There was a problem hiding this comment.
I applied the same change to this new file.
|
Before merging, let’s compare this with the approach of |
|
Preferring #608. |
naure: