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

chore: refactor air in keccak to not use offset_of #308

Merged
merged 4 commits into from Feb 24, 2024
Merged

Conversation

tamirhemo
Copy link
Contributor

Remove the plonky3 Keccak air columns from the column struct so that we don't have to worry about offset.

@tamirhemo tamirhemo changed the title chore: refactor air in keccak to not use offset_of fix: refactor air in keccak to not use offset_of Feb 24, 2024
Copy link
Contributor

@kevjue kevjue left a comment

Choose a reason for hiding this comment

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

LGTM. Please take a look at the nit comments.

pub const NUM_KECCAK_COLS: usize = size_of::<KeccakCols<u8>>();
pub const P3_KECCAK_COLS_OFFSET: usize = offset_of!(KeccakCols<u8>, p3_keccak_cols);
pub const NUM_KECCAK_MEM_COLS: usize = size_of::<KeccakMemCols<u8>>();
// pub const P3_KECCAK_COLS_OFFSET: usize = offset_of!(KeccakCols<u8>, p3_keccak_cols);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove comment?


use self::columns::P3_KECCAK_COLS_OFFSET;
// use self::columns::P3_KECCAK_COLS_OFFSET;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove?

@tamirhemo tamirhemo changed the title fix: refactor air in keccak to not use offset_of chore: refactor air in keccak to not use offset_of Feb 24, 2024
@tamirhemo tamirhemo merged commit 5bfc51e into main Feb 24, 2024
5 checks passed
@tamirhemo tamirhemo deleted the tamir/keccak branch February 24, 2024 18:18
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.

None yet

2 participants