Add support for CRC64, refactor CRC32 #3177
Conversation
|
|
||
| static std::shared_ptr<HashFactory>& GetCRC64Factory() | ||
| { | ||
| static std::shared_ptr<HashFactory> s_CRC64Factory(nullptr); |
There was a problem hiding this comment.
how does it work with our custom memory management?
There was a problem hiding this comment.
its gets assigned when we call its factory, it follows the same call path as CRC32 does. i.e.
create accessor for static factory
static std::shared_ptr<HashFactory>& GetCRC32Factory()
{
static std::shared_ptr<HashFactory> s_CRC32Factory(nullptr);
return s_CRC32Factory;
}then is assigned during the init crypto call
if(!GetCRC32Factory())
{
GetCRC32Factory() = Aws::MakeShared<DefaultCRC32Factory>(s_allocationTag);
}then is set to nullptr before deleting
if(GetCRC32CFactory())
{
GetCRC32Factory() = nullptr;
}There was a problem hiding this comment.
yeah changing this pattern and avoiding static state would be a larger change than this, but I agree something that should be done as part of some future "eliminate static state" task
sbiscigl
force-pushed
the
crc64
branch
2 times, most recently
from
294b25b to
986461e
Compare
| m_hashImpl->Update(buffer, bufferSize); | ||
| } | ||
|
|
||
| HashResult CRC64::GetHash() |
There was a problem hiding this comment.
get apis typically should be marked const for correctness
There was a problem hiding this comment.
cant because the original API is not, and we cannot change it without breaking backwards compatibilty.
| uint8_t streamBuffer[Aws::Utils::Crypto::Hash::INTERNAL_HASH_STREAM_BUFFER_SIZE]; | ||
| while (stream.good()) | ||
| { | ||
| stream.read(reinterpret_cast<char*>(streamBuffer), Aws::Utils::Crypto::Hash::INTERNAL_HASH_STREAM_BUFFER_SIZE); |
There was a problem hiding this comment.
better to use
while (stream.read(reinterpret_cast<char*>(streamBuffer), Aws::Utils::Crypto::Hash::INTERNAL_HASH_STREAM_BUFFER_SIZE)) {
to handle end of stream errors more cleanly
There was a problem hiding this comment.
this actually causes the integration tests to fail, so there is a behavior difference between stream.good and if(stream.read). this is also how it is currently done. this is just moving it to a non copied pasted place
|
to format code , please use the following cmd |
| } | ||
|
|
||
| void CRC64::Update(unsigned char* buffer, size_t bufferSize) | ||
| { |
There was a problem hiding this comment.
null check?
also constness of the ptr, unsigned char* const buffer ?
There was a problem hiding this comment.
can add null check, cant change constness because the original API is not, and we cannot change it without breaking backwards compatibilty.
you didnt commit a |
using the clang-format-diff specifying the style doesn't need the clang_format file. On can provide the file for customizations but its not needed per documentation |
we're formatting against a unknown default format then, im against this check until we add a defined file, otherwise we dont know what style we are formatting against as none is specified. |
sbiscigl
force-pushed
the
crc64
branch
2 times, most recently
from
21e631c to
8d0703d
Compare
|
lgtm |
3 participants
Description of changes:
Adds a crypto utility class for CRC64 from CRT, refactors CRC32 related crypto factories to avoid DRY-ing.
Check all that applies:
Check which platforms you have built SDK on to verify the correctness of this PR.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.