Skip to content

Refactor: Remove MDx_HashFunction#3705

Merged
reneme merged 9 commits intorandombit:masterfrom
Rohde-Schwarz:refactor/remove_mdx_base
Oct 5, 2023
Merged

Refactor: Remove MDx_HashFunction#3705
reneme merged 9 commits intorandombit:masterfrom
Rohde-Schwarz:refactor/remove_mdx_base

Conversation

@reneme
Copy link
Copy Markdown
Collaborator

@reneme reneme commented Sep 18, 2023 •
edited
Loading

Pull Request Dependencies

Description

This is a re-iteration of #3550 which became stale mostly due to clang-format. Note that I took the liberty to implement the alternative suggestion on configuring the instances' static configurations. I posted this on the original pull request: #3550 (comment).

In my opinion, this has two important advantages:

  1. readability: All "magic numbers" in the MDx instances have clear names, instead of just being an unordered template parameter list. Also, those names are available throughout the hash's and the helper's implementation.
  2. "global" static availability: The "magic numbers" are available from anywhere within the library. For instance: If some internal downstream code needs the output length of SHA-256 as a static information, it can just include sha2_32.h and go for SHA_256::output_bytes.

@randombit I'd be interested in your opinion on the usefulness of exposing some algorithm specifics statically. FWIW: Here's another example where this would be helpful: #3609 (comment). To be clear: I think, this should be an internal feature and not exposed via public API.

@reneme reneme mentioned this pull request Sep 18, 2023
Closed
@reneme reneme force-pushed the refactor/remove_mdx_base branch from febaf6a to 0548e7d Compare September 18, 2023 15:18
@coveralls
Copy link
Copy Markdown

coveralls commented Sep 18, 2023 •
edited
Loading

Coverage Status

coverage: 91.753% (+0.004%) from 91.749% when pulling 4c45536 on Rohde-Schwarz:refactor/remove_mdx_base into 1f26e75 on randombit:master.

@reneme reneme force-pushed the refactor/remove_mdx_base branch 2 times, most recently from 54c4f89 to 4c45536 Compare September 19, 2023 06:17
@reneme
Copy link
Copy Markdown
Collaborator Author

reneme commented Sep 19, 2023

Rebased to master after #3693 got merged.

@reneme reneme mentioned this pull request Sep 19, 2023
Open
5 tasks
@reneme reneme mentioned this pull request Sep 26, 2023
Open
47 tasks
randombit
randombit reviewed Sep 29, 2023
View reviewed changes
Copy link
Copy Markdown
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.

I haven't done a complete review yet, but yeah looks pretty good.

Please do check that this doesn't introduce performance regressions.

@randombit randombit added this to the Botan 3.2.0 milestone Sep 29, 2023
@randombit
Copy link
Copy Markdown
Owner

randombit commented Oct 4, 2023

On my machine anyway this seems very slightly slower, but it's under .1 cycles per byte and mostly notable for MD4/MD5 where the compression function is very cheap - seems fine.

randombit
randombit reviewed Oct 4, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Did a full review, this is basically fine outside of some quibbling about what should or should not be inline.

@reneme reneme force-pushed the refactor/remove_mdx_base branch from 4c45536 to 3e61eba Compare October 5, 2023 07:18
@reneme
Copy link
Copy Markdown
Collaborator Author

reneme commented Oct 5, 2023

Thanks for the review! I'm glad you like this design.

I'm done with addressing your comments. I.e. renaming the MD hash template variable from T to MD and moving the implementations of copy_state, new_object, add_data and final_result into the respective *.cpp files of the concrete hash implementations.

randombit
randombit approved these changes Oct 5, 2023
View reviewed changes
Copy link
Copy Markdown
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.

Thanks @reneme looks great

@reneme reneme merged commit 1db2b19 into randombit:master Oct 5, 2023
@reneme reneme deleted the refactor/remove_mdx_base branch October 5, 2023 12:16
@FAlbertDev FAlbertDev mentioned this pull request Oct 9, 2023
Merged
@reneme reneme mentioned this pull request Jul 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@randombit randombit randombit approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Botan 3.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@reneme @coveralls @randombit