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

Utility: Minor improvements to Sha1 and AbstractHash #85

Closed
wants to merge 6 commits into from

Conversation

Squareys
Copy link
Contributor

Hi @mosra !

As discussed via gitter, here is ArrayView input support for Sha1 << and the removal of DebugStl.h from AbstractHash.

Cheers,
Jonathan

src/Corrade/Utility/Sha1.h Show resolved Hide resolved
src/Corrade/Utility/Sha1.cpp Outdated Show resolved Hide resolved
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@codecov-io
Copy link

codecov-io commented Dec 22, 2019

Codecov Report

Merging #85 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   97.65%   97.68%   +0.02%     
==========================================
  Files          89       89              
  Lines        6063     6180     +117     
==========================================
+ Hits         5921     6037     +116     
- Misses        142      143       +1
Impacted Files Coverage Δ
src/Corrade/Utility/Sha1.h 100% <ø> (ø) ⬆️
src/Corrade/Utility/Sha1.cpp 100% <100%> (ø) ⬆️
src/Corrade/Utility/AbstractHash.h 96.55% <100%> (ø) ⬆️
src/Corrade/Utility/FileWatcher.cpp 97.87% <0%> (-2.13%) ⬇️
src/Corrade/TestSuite/Tester.h 100% <0%> (ø) ⬆️
src/Corrade/Interconnect/Emitter.cpp 100% <0%> (ø) ⬆️
src/Corrade/Utility/Format.h 100% <0%> (ø) ⬆️
.../Corrade/TestSuite/Implementation/BenchmarkStats.h 100% <0%> (ø) ⬆️
src/Corrade/Utility/Implementation/tweakable.h 100% <0%> (ø) ⬆️
src/Corrade/TestSuite/Comparator.cpp 100% <0%> (ø) ⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0280fbe...bf7c1e3. Read the comment docs.

@Squareys
Copy link
Contributor Author

@mosra I addressed the feedback and moved the code to use _buffer[128] instead of std::string.

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

There's one critical issue and a few minor things, but apart from that looks good I guess :)

src/Corrade/Utility/Sha1.h Outdated Show resolved Hide resolved
src/Corrade/Utility/Test/Sha1Test.cpp Outdated Show resolved Hide resolved
src/Corrade/Utility/Sha1.h Outdated Show resolved Hide resolved
src/Corrade/Utility/Sha1.cpp Outdated Show resolved Hide resolved
src/Corrade/Utility/Test/Sha1Test.py Show resolved Hide resolved
src/Corrade/Utility/Sha1.h Outdated Show resolved Hide resolved
Signed-off-by: Squareys <squareys@googlemail.com>
Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

@mosra Done. Thanks for all the feedback!

I also added a code snippet for the usage, I find those particularly useful when searching through the docs.

class CORRADE_UTILITY_EXPORT Sha1: public AbstractHash<20> {
public:
/**
* @brief Digest of given data
*
* Convenience function for @cpp (Utility::Sha1{} << data).digest() @ce.
* The @cpp '\0' @ce delimiter is not included.
Copy link
Owner

Choose a reason for hiding this comment

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

I wouldn't mention this here at all, it's misleading -- what it does do when your string has a 0 byte in the middle? Or when data.back() == '\0'? The std::string historically didn't have to allocate extra space for the 0-delimiter (it does so only from c++11).

Same below and in the snippet above, it only adds confusion IMHO -- it takes all size() bytes from it, which is what one expects, so it doesn't need to be mentioned explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It cause a lot of confusion for me not knowing whether or not it does include the final delimiter and I originally expected it to, so having this somewhere would make sense to me... maybe as a note? If there is \0 in the middle, it will already mess up there with the char* => string conversion and use strlen... maybe add a note for that? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this line.

Copy link
Owner

Choose a reason for hiding this comment

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

If there is \0 in the middle, it will already mess up there with the char* => string conversion and use strlen

Nope, you can do std::string{"ab\0cde"} and that'll make the string six characters long with a zero in the middle.

doc/snippets/Utility.cpp Show resolved Hide resolved
Squareys and others added 2 commits December 23, 2019 20:08
Signed-off-by: Squareys <squareys@googlemail.com>
Co-authored-by: mosra <mosra@centrum.cz>
…icit

Signed-off-by: Squareys <squareys@googlemail.com>
@Squareys
Copy link
Contributor Author

@mosra I made those two changes (see answers to those comments).

@mosra mosra added this to the 2020.0a milestone Dec 30, 2019
@mosra mosra added this to TODO in Utility via automation Dec 30, 2019
@mosra
Copy link
Owner

mosra commented Dec 30, 2019

Merged with a bunch of minor doc clarifications / commit squashing in 004f974...f627677. Thank you!

@mosra mosra closed this Dec 30, 2019
Utility automation moved this from TODO to Done Dec 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Utility
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants