Refactorings needed for injection support#186
Open
eclipse-impl wants to merge 1 commit into
Open
Conversation
|
The created documentation from the pull request is available at: docu-html |
There was a problem hiding this comment.
Pull request overview
This PR prepares the SCORE hash library for client-side extension/injection of hashing functionality (including proprietary implementations) by widening Bazel visibility and introducing clearer extension points around CRC variants.
Changes:
- Expose selected hash-related Bazel targets publicly to enable external composition/overrides.
- Refactor CRC32 IEEE implementation to share a reusable compile-time lookup table and remove
custom.bzlhooks. - Rename
HashAlgorithm::kCrc32UnusedtoHashAlgorithm::kCrc32Autosarand update factories/tests/logging accordingly.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| score/hash/code/sha256digest/BUILD | Makes sha256digest library publicly visible for external wiring. |
| score/hash/code/openssl/openssl_wrapper/BUILD | Makes OpenSSL mock target publicly visible (for external tests). |
| score/hash/code/openssl/openssl_hash_calculator.cpp | Updates unsupported algorithm switch cases for renamed CRC enum. |
| score/hash/code/openssl/BUILD | Makes OpenSSL calculator library publicly visible for injection scenarios. |
| score/hash/code/crc/lookup_table.h | Adds shared constexpr CRC lookup table implementation. |
| score/hash/code/crc/custom.bzl | Removes no-op custom hook file. |
| score/hash/code/crc/crc32_ieee.cpp | Switches CRC32 IEEE to use shared lookup table header. |
| score/hash/code/crc/BUILD | Exposes CRC IEEE library publicly and documents extension approach. |
| score/hash/code/core/factory/impl/safe_hash_calculator_factory_ieee.cpp | Updates unsupported algorithm cases for renamed CRC enum. |
| score/hash/code/core/factory/impl/hash_calculator_factory_ieee.cpp | Updates unsupported algorithm cases for renamed CRC enum. |
| score/hash/code/core/factory/impl/hash_calculator_factory_ieee_test.cpp | Updates test to use renamed CRC algorithm (but test name still says “Unused”). |
| score/hash/code/core/factory/impl/custom.bzl | Removes no-op custom hook file. |
| score/hash/code/core/factory/impl/BUILD | Adds a public header-only target and updates extension-point documentation. |
| score/hash/code/common/algorithms.h | Renames CRC enum value and size constant; updates logging string. |
| score/hash/code/common/algorithms.cpp | Updates size mapping and identification comments for renamed CRC enum. |
| score/hash/code/common/algorithms_test.cpp | Updates tests for renamed CRC enum and log output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
+59
| #include <cstdint> | ||
|
|
||
| namespace score | ||
| { | ||
| namespace hash | ||
| { | ||
| namespace internal | ||
| { | ||
|
|
||
| /// @brief Compile-time CRC32 lookup table parameterised by a reflected polynomial. | ||
| /// | ||
| /// This is an implementation detail shared by CRC32 calculator classes. | ||
| /// It is not part of the public API. | ||
| template <std::uint_fast32_t ReversePolynomial> | ||
| class LookupTable final | ||
| { | ||
| public: | ||
| constexpr LookupTable() : table_{} | ||
| { | ||
| for (std::uint_fast32_t table_index = 0U; table_index < kTableSize; ++table_index) | ||
| { | ||
| auto checksum = table_index; | ||
|
|
||
| for (auto round = 0U; round < 8U; ++round) | ||
| { | ||
| if (static_cast<bool>(checksum & 0x1U)) | ||
| { | ||
| checksum = (checksum >> 1U) ^ ReversePolynomial; | ||
| } | ||
| else | ||
| { | ||
| checksum = (checksum >> 1U) ^ 0U; | ||
| } | ||
| } | ||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) can't use .at() in constexpr fn | ||
| table_[table_index] = checksum; | ||
| } | ||
| } | ||
|
|
||
| constexpr std::uint_fast32_t operator[](size_t i) const noexcept | ||
| { | ||
| // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) can't use .at() in constexpr fn | ||
| return table_[i]; |
Comment on lines
+18
to
+21
| # If one needs to provide different polynomials, one can directly inherit | ||
| # from this target and use the `label_flag` at: | ||
| # score/hash/code/core/factory/impl/BUILD | ||
| # For providing the extended version as a replacement for `(safe_)crc_variant`. |
Comment on lines
+74
to
+79
| # meaning that one may replace them by custom implementations that would | ||
| # provide different polynomials. These extensions may directly inherit from: | ||
| # //score/hash/code/crc:crc_ieee | ||
| # As that target provides a generic implementation of CRC, the custom | ||
| # implementations would then only need to provide their polynomials | ||
| # and override these two targets on their .bazelrc files. |
Comment on lines
+107
to
+113
| score::cpp::span<const std::uint8_t> data(test_input); | ||
|
|
||
| auto result = unit.CalculateHash(HashAlgorithm::kCrc32Unused, data); | ||
| auto result = unit.CalculateHash(HashAlgorithm::kCrc32Autosar, data); |
| kSha512, | ||
| kCrc32, | ||
| kCrc32Unused, | ||
| kCrc32Autosar, |
Contributor
Author
There was a problem hiding this comment.
There are currently no users for the API, so it's fine to break it.
Different SCORE users might have specific needs for supporting proprietary or copyrighted hashing functions. Therefore we need to provide the mechanism for clients injecting their extensions to the hash lib. This commit prepares the open-source code for the injection of a copyrighted hash function by one of the clients. Notice that the aim is to achieve a design that will be sufficiently generic that will allow for extensions without any change on the open-source side.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Different SCORE users might have specific needs for supporting proprietary or copyrighted hashing functions. Therefore we need to provide the mechanism for clients injecting their extensions to the hash lib.
This commit prepares the open-source code for the injection of a copyrighted hash function by one of the clients.
Notice that the aim is to achieve a design that will be sufficiently generic that will allow for extensions without any change on the open-source side.