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: Buffered_Computation can deal with generic containers #3294

Merged
merged 4 commits into from
Mar 17, 2023

Conversation

reneme
Copy link
Collaborator

@reneme reneme commented Feb 16, 2023

This should be a transparent change for the most part but enables modern support for all sorts of containers without resorting to the C-style pointer APIs.

Allows passing all sorts of contiguous ranges to ::update() and ::process() as well as storing results of Buffered_Computation in user-defined contiguous containers.

The latter is most-useful for the strong types:

using MessageHash = Strong<std::vector<uint8_t>, struct MessageHash_>;

auto sha = HashFunction::create("SHA-256");
sha->update("some message");
auto message_hash = sha->final<MessageHash>();

This should be a transparent change for the most part but
enables modern support for all sorts of containers without
resorting to the C-style pointer APIs
@reneme
Copy link
Collaborator Author

reneme commented Feb 16, 2023

Sorry, I should have marked this as a draft while I was still working on it.

We added another overload for ::process<>() that allows custom output containers (same as ::final<>()). And also there's a test now, that makes sure all this convenience is actually working (aka compiling).

@reneme reneme requested a review from randombit February 16, 2023 16:42
@reneme
Copy link
Collaborator Author

reneme commented Feb 16, 2023

I deliberately left the virtual methods (below) untouched to get the low hanging fruits first.

virtual void add_data(const uint8_t input[], size_t length) = 0;
virtual void final_result(uint8_t out[]) = 0;

Though, both are good candidates for std::span<>. Particularly with sub-classes being forced to trust their base class(s) and write into an unbounded array. 😨

What's your take on this? Should we take this on?

@codecov-commenter
Copy link

Codecov Report

Base: 88.08% // Head: 88.09% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (c4582aa) compared to base (e1b3226).
Patch coverage: 98.30% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3294      +/-   ##
==========================================
+ Coverage   88.08%   88.09%   +0.01%     
==========================================
  Files         610      611       +1     
  Lines       68481    68577      +96     
  Branches     6830     6832       +2     
==========================================
+ Hits        60322    60414      +92     
- Misses       5303     5310       +7     
+ Partials     2856     2853       -3     
Impacted Files Coverage Δ
src/tests/test_bufcomp.cpp 98.30% <98.30%> (ø)
src/lib/pbkdf/argon2/argon2pwhash.cpp 69.69% <0.00%> (-9.10%) ⬇️
src/lib/utils/cpuid/cpuid_x86.cpp 54.05% <0.00%> (-0.62%) ⬇️
src/lib/math/numbertheory/numthry.cpp 83.81% <0.00%> (-0.58%) ⬇️
src/lib/tls/tls12/tls_record.cpp 91.80% <0.00%> (-0.41%) ⬇️
src/lib/block/aes/aes_ni/aes_ni.cpp 99.80% <0.00%> (-0.20%) ⬇️
src/lib/math/numbertheory/dsa_gen.cpp 89.28% <0.00%> (-0.19%) ⬇️
src/tests/test_hash.cpp 84.96% <0.00%> (-0.10%) ⬇️
src/tests/test_bigint.cpp 90.81% <0.00%> (-0.03%) ⬇️
src/lib/block/sm4/sm4.cpp 100.00% <0.00%> (ø)
... and 12 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@randombit
Copy link
Owner

What's your take on this? Should we take this on?

Sure, using span internally here would be a lot cleaner overall. But also a lot of work since you'll have to touch every hash/MAC. Those kind of changes tend to be a slog.

@reneme
Copy link
Collaborator Author

reneme commented Feb 24, 2023

@randombit I didn't merge this yet because I pushed some API changes after you approved the PR. Most notably: c4582aa

@reneme reneme mentioned this pull request Mar 16, 2023
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

👍

@reneme reneme merged commit 8460ca2 into master Mar 17, 2023
@randombit randombit deleted the chore/generic_interface_bufcomp branch March 21, 2023 22:21
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.

3 participants