-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
src: minor cleanup and simplification of crypto::Hash #35651
Conversation
Signed-off-by: James M Snell <jasnell@gmail.com>
Review requested:
|
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Micro-nit that can totally be ignored: First word after the subsystem in the commit message should be an imperative verb. Maybe src: simplify crypto::Hash implementation
?
LGTM but I'm going to wait for a domain expert to weigh in first....
This comment has been minimized.
This comment has been minimized.
That's really weird, I am sure I've been in that team for years. Thanks for the ping. |
I may have just overlooked your name in the list. |
Landed in 1103b15...84a7880 |
Signed-off-by: James M Snell <jasnell@gmail.com> PR-URL: #35651 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Rich Trott <rtrott@gmail.com>
Some minor cleanups and simplification for
crypto::Hash
@addaleax @targos... I was working on this because
valgrind --leak-check=full
is reporting the following whencrypto.createHash()
is called but I can't seem to be able to track down the uninitialized memory...After digging in, I've tracked down the above warning to the HashInit call inside Hash::New ... But, here's the fun part, adding any printf inside HashInit makes the initialized jump/move disappear.
This depends on the recent semver-major crypto refactor but can be backported.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes