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: BufferTransformer #3942

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Mar 25, 2024

Pull Request Dependencies

Description

This is in response of #3885 (comment): Combining the BufferStuffer and BufferSlicer into a BufferTransformer that always acts on two buffers (one input, one output) in lock-step. Block ciphers (and in particular implementations of BlockCipher::encrypt_n()/::decrypt_n()) are a typical use case for this helper.

I added this spike on #3870, which introduced std::span in the relevant methods, to simplify the integration. See Rohde-Schwarz@00ff565 for the relevant patch.

@randombit Is that what you had in mind?

This demotes Block_Cipher::encrypt_n/decrypt_n to top-level methods
and introduces new (private) virtual methods (encrypt/decrypt_blocks)
that use std::span for the in/out buffers. Also, this adapts all block
cipher implementations in the library to use the new API.
@reneme reneme requested a review from randombit March 25, 2024 16:24
@coveralls
Copy link

Coverage Status

coverage: 92.086% (-0.005%) from 92.091%
when pulling 00ff565 on Rohde-Schwarz:chore/buffer_transformer
into 38a0b56 on randombit:master.

@randombit
Copy link
Owner

Is that what you had in mind?

Basically so, yeah. I wonder if we can fit in some other common functionality there, namely the word level loads/stores. That's a very common task (encrypt+decrypt for N ciphers plus alternative implementations) so if there is a nice way to make that work it would cut some complexity. Not necessary for the initial PR. I just found an explict slicer+stuffer quite verbose in the original change.

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

3 participants