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.

Sign up for GitHub

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

Jump to bottom

Plonk: code cleanup #63

Merged
merged 1 commit into from
Aug 18, 2022
Merged

Plonk: code cleanup #63

merged 1 commit into from
Aug 18, 2022

Conversation

vesselinux
Copy link
Collaborator

Addresses issue #58

@vesselinux vesselinux changed the title Plonk code cleanup Plonk: code cleanup Jul 26, 2022
@vesselinux vesselinux requested a review from dtebbs July 29, 2022 08:07
@dtebbs
Copy link
Contributor

dtebbs commented Jul 29, 2022

This seems ilke it should target the #62 branch. Otherwise the Files changed tab includes changes ffrom all PRs and it's difficult to review.

@vesselinux vesselinux force-pushed the plonk-code-cleanup-issue-58 branch 2 times, most recently from 4aeb838 to 9650199 Compare July 29, 2022 14:58
@vesselinux
Copy link
Collaborator Author

This seems ilke it should target the #62 branch. Otherwise the Files changed tab includes changes ffrom all PRs and it's difficult to review.

Rebased on plonk and dropped all commits not relevant to this one.

dtebbs
dtebbs approved these changes Aug 16, 2022
View reviewed changes
Copy link
Contributor

@dtebbs dtebbs left a comment

Choose a reason for hiding this comment

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

LGTM

@dtebbs
Copy link
Contributor

dtebbs commented Aug 16, 2022

I think this needs rebasing onto origin/plonk, then ready to merge.

@vesselinux
Copy link
Collaborator Author

vesselinux commented Aug 16, 2022 •
edited
Loading

I think this needs rebasing onto origin/plonk, then ready to merge.

Hm, I thought it's already rebased cf. #63 (comment) . Am I wrong?

Regarding the merge, what is the usual practise: should I merge it or you?

plonk: fixed functions plonk_compute_accumulator and plonk_compute_se…
0dcea63 
…lector_polynomials to allocate the size of their return values inside of the function i.e. not to rely on the caller to pass inputs with correct size. addressing issue #58, bullet 7
@vesselinux vesselinux force-pushed the plonk-code-cleanup-issue-58 branch from 9650199 to 0dcea63 Compare August 17, 2022 19:46
@vesselinux
Copy link
Collaborator Author

Hm, I thought it's already rebased cf. #63 (comment) . Am I wrong?

I was wrong indeed. I hadn't updated my repo with the merge of PR #60 when creating this branch off plonk. Rebased on plonk now so it's fixed.

@vesselinux vesselinux requested a review from dtebbs August 17, 2022 20:22
@dtebbs dtebbs merged commit 1ab4e80 into plonk Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtebbs dtebbs Awaiting requested review from dtebbs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vesselinux @dtebbs