Skip to content
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

fix: review the pub types/fields(post-split)(1) #307

Merged
merged 10 commits into from
Apr 16, 2024
Merged

Conversation

duguorong009
Copy link

@duguorong009 duguorong009 commented Apr 3, 2024

Description

  • review & correct the pub types/fields in every crate

Related issues

Changes

  • add (crate) limit to needed types/fields in every crate
  • remove the unused methods
  • add #[allow(dead_code)] for only necessary case, like Debug-derived structs(PinnedVerificationKey, ...)

Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think there should be no #[allow(dead_code)] anywhere, because it's only needed for non-pub entries, but if they're not public and they're not used anywhere, why keep them?

halo2_backend/src/helpers.rs Outdated Show resolved Hide resolved
halo2_backend/src/plonk.rs Outdated Show resolved Hide resolved
halo2_backend/src/plonk/verifier/batch.rs Outdated Show resolved Hide resolved
halo2_backend/src/poly.rs Outdated Show resolved Hide resolved
halo2_frontend/src/circuit/floor_planner/single_pass.rs Outdated Show resolved Hide resolved
@@ -85,26 +85,31 @@ impl Region {
}

/// Returns the name of the region.
#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's inconsistent that Region is not pub but it has pub methods.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the mehods in 82ff602

halo2_middleware/src/expression.rs Outdated Show resolved Hide resolved
halo2_middleware/src/expression.rs Outdated Show resolved Hide resolved
halo2_middleware/src/expression.rs Outdated Show resolved Hide resolved
Copy link

@davidnevadoc davidnevadoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ed255, we shouldn't have #[allow(dead_code)], these functions should be removed or remain pub.
The purpose of this PR is to restore function visibility to the state be fore the front/back split right? ( I assume that during the process, the visibility for many function was increased for convenience.)
If this is the case, I suggest we leave thing as pub when in doubt.

halo2_backend/src/poly.rs Outdated Show resolved Hide resolved
halo2_middleware/src/expression.rs Outdated Show resolved Hide resolved
@duguorong009
Copy link
Author

duguorong009 commented Apr 4, 2024

Thanks for the detailed review! @ed255 @davidnevadoc

I followed the suggestions - roll back some pub & remove the unused methods.
Pls re-check the PR.

Copy link
Member

@ed255 ed255 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@duguorong009 duguorong009 merged commit bd385c3 into main Apr 16, 2024
15 checks passed
@duguorong009 duguorong009 deleted the gr@review-pub-1 branch April 16, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[post frontend-backend split] Review pub types / fields
3 participants