chore: break big files into smaller ones

guorong009 commented 4 months ago

Description

Break the big files(> 1k LOC) into smaller files

Related issues

Resolve #297

Changes

refactor the files in halo2_frontend/src/plonk/circuit dir
- create new files - ../plonk/circuit/helpers.rs & ../plonk/circuit/constraint_system/helpers.rs
- move the contents from expression.rs & constraint_system.rs to new files
refactor the halo2_frontend/src/circuit.rs file
- create new file witness_collector.rs inside halo2_frontend/src/circuit/ dir

NOTE

There are additional big files - halo2_frontend/src/dev.rs & halo2_backend/src/plonk/prover.rs. I believe they don't need splitting, since they include only what's needed.

guorong009 commented 4 months ago

Most of the changes look good to me but I have two comments:

The first is that I see the new files column.rs, phase.rs and query.rs which have definitions that were previously in expression.rs. I notice that these new files are very small. Could they be merged in a single file?

The other comment is related to plonk/circuit.rs. To me it feels strange to move the types Constraint, Constraints, SelectorsToFixed, Gate and so on, which are types directly used by the ConstraintSystem at plonk/circuit/constraint_system.rs into plonk/circuits.rs which is a more general module. In my mind, the more general things are in less deep modules, and more specific things are in more deep modules; so it's strange to see that types that are used specifically by plonk/circuit/constraint_system.rs are found in plonk/circuit.rs. I think I would prefer moving those to plonk/circuit/constraint_system/foo.rs instead.

Please let me know your thoughts about these two points. To me the second one is more important to discuss and resolve; the first one could be more related to personal taste.

Thanks for suggestions! @ed255 1) I believe it is right to create less new files. Hence, I remove 3 new files & create plonk/circuit/helpers.rs file for Expression -related contents.

2) I also think you are right. FYI, I originally moved out all of those structs to plonk/circuit.rs, because they are also exported & used in halo2_proofs crate. But, it seems that there is no need. I create a new file plonk/circuit/constraint_system/helpers.rs for ConstraintSystem-related contents.

Please check new updates & give more feedback.

guorong009 commented 4 months ago

Thanks for the review! @ed255

I think this PR is a bit awkward since it tries to split the code in unnatural way. It just aims to reduce the LOC, and breaks the cohesion. Plus, a new structure is not that good. How about closing the PR, without merge? (Maybe, close the issue also?)

ed255 commented 4 months ago

I think this PR is a bit awkward since it tries to split the code in unnatural way. It just aims to reduce the LOC, and breaks the cohesion. Plus, a new structure is not that good. How about closing the PR, without merge? (Maybe, close the issue also?)

Closing the PR and the issue sounds good to me!

guorong009 commented 4 months ago

Close the PR since it leads to unnatural, incoherent refactoring. Plus, close #297 as resolved.

privacy-scaling-explorations / halo2