Clang-tidy fix clp::ffi namespace top level folder files and related test cases #509
Conversation
|
@Bill-hbrhbr, can you fix the unit test failure before the review? |
There was a problem hiding this comment.
This will be the first round of the review. Talking to Kirk and we decided to also apply the latest coding standards even though they might not trigger a clang-tidy warning. This means there should be more stuff to be fixed in this PR. At some point we might need to draw the line that what are the stuff we want to include in this PR, but I guess we can start with these current comments. Note that some comments should be propagated to all the files, such as using {} for initialization whenever possible.
| class EncodingException : public TraceableException { | ||
| public: | ||
| // Constructors | ||
| EncodingException( | ||
| ErrorCode error_code, | ||
| char const* const filename, | ||
| int line_number, | ||
| std::string message | ||
| ) | ||
| : TraceableException(error_code, filename, line_number), | ||
| m_message(std::move(message)) {} | ||
|
|
||
| // Methods | ||
| [[nodiscard]] auto what() const noexcept -> char const* override { return m_message.c_str(); } | ||
|
|
||
| private: | ||
| std::string m_message; | ||
| }; |
There was a problem hiding this comment.
How about moving this class to a dedicated header?
| : TraceableException(error_code, filename, line_number), | ||
| m_message(std::move(message)) {} |
There was a problem hiding this comment.
| : TraceableException(error_code, filename, line_number), | |
| m_message(std::move(message)) {} | |
| : TraceableException{error_code, filename, line_number}, | |
| m_message{std::move(message)} {} |
| /* | ||
| * These constants can be used by callers to store the version of the schemas and encoding methods | ||
| * they're using. At some point, we may update and/or add built-in schemas/encoding methods. So | ||
| * callers must store the versions they used for encoding to ensure that they can choose the same | ||
| * versions for decoding. | ||
| * | ||
| * We use versions which look like package names in anticipation of users writing their own custom | ||
| * schemas and encoding methods. | ||
| */ | ||
| constexpr std::string_view cVariableEncodingMethodsVersion | ||
| = "com.yscope.clp.VariableEncodingMethodsV1"; | ||
| constexpr std::string_view cVariablesSchemaVersion = "com.yscope.clp.VariablesSchemaV2"; | ||
|
|
||
| constexpr std::string_view cTooFewDictionaryVarsErrorMessage | ||
| = "There are fewer dictionary variables than dictionary variable placeholders in the " | ||
| "logtype."; | ||
| constexpr std::string_view cTooFewEncodedVarsErrorMessage | ||
| = "There are fewer encoded variables than encoded variable placeholders in the logtype."; | ||
| constexpr std::string_view cUnexpectedEscapeCharacterMessage | ||
| = "Unexpected escape character without escaped value at the end of the logtype."; | ||
| constexpr std::string_view cTooManyDigitsErrorMsg = "Encoded number of digits doesn't match " | ||
| "encoded digits in encoded float."; | ||
|
|
||
| constexpr size_t cMaxDigitsInRepresentableEightByteFloatVar = 16; | ||
| constexpr size_t cMaxDigitsInRepresentableFourByteFloatVar = 8; | ||
| constexpr uint64_t cEightByteEncodedFloatNumDigits = 54; | ||
| constexpr uint64_t cFourByteEncodedFloatNumDigits = 25; | ||
| constexpr uint64_t cEightByteEncodedFloatDigitsBitMask = (1ULL << 54) - 1; | ||
| constexpr uint32_t cFourByteEncodedFloatDigitsBitMask = (1UL << 25) - 1; | ||
|
|
||
| constexpr size_t cDecimalBase = 10; | ||
| constexpr uint32_t cLowerFourDigitsBitMask = (1UL << 4) - 1; | ||
| constexpr uint32_t cLowerThreeDigitsBitMask = (1UL << 3) - 1; |
There was a problem hiding this comment.
Can we use {} for initialization instead?
| } // namespace clp::ffi | ||
|
|
||
| // TODO Refactor nested headers |
There was a problem hiding this comment.
| // TODO Refactor nested headers | |
| // TODO: Refactor nested headers |
Just wanna make sure: you will send a PR immediately after this one to get rid of .inc right?
| uint8_t num_high_bits | ||
| = std::is_same_v<TestType, four_byte_encoded_variable_t> ? 1 : 2; | ||
| for (size_t high_bits = 0; high_bits < num_high_bits; ++high_bits) { | ||
| for (size_t high_bits{0}; high_bits < num_high_bits; ++high_bits) { | ||
| TestType test_encoded_var; | ||
| if (std::is_same_v<TestType, eight_byte_encoded_variable_t>) { |
There was a problem hiding this comment.
| if (std::is_same_v<TestType, eight_byte_encoded_variable_t>) { | |
| if constexpr (std::is_same_v<TestType, eight_byte_encoded_variable_t>) { |
| uint8_t num_high_bits | ||
| = std::is_same_v<TestType, four_byte_encoded_variable_t> ? 1 : 2; | ||
| for (size_t high_bits = 0; high_bits < num_high_bits; ++high_bits) { | ||
| for (size_t high_bits{0}; high_bits < num_high_bits; ++high_bits) { | ||
| TestType test_encoded_var; |
There was a problem hiding this comment.
| TestType test_encoded_var; | |
| TestType test_encoded_var{}; |
| @@ -336,8 +266,8 @@ TEMPLATE_TEST_CASE( | |||
| ); | |||
| // Since encode_float_properties erases the low bit of high_bits, we need to | |||
| // add it again manually | |||
| test_encoded_var | |||
| = (high_bits << 62) | (((1ULL << 62) - 1) & test_encoded_var); | |||
| test_encoded_var = (high_bits << num_low_bits) | |||
There was a problem hiding this comment.
The result is an unsigned integer no? I think we should use bit_cast for safety reasons
| all_dictionary_vars.append(message.data() + begin_pos, message.data() + end_pos); | ||
| dictionary_var_end_offsets.push_back(all_dictionary_vars.length()); | ||
| all_dictionary_vars.append(message, begin_pos, end_pos - begin_pos); | ||
| dictionary_var_end_offsets.emplace_back(all_dictionary_vars.length()); |
There was a problem hiding this comment.
For fundamental types let's use push_back since no in-place constructors are needed
| size_t all_dictionary_vars_length = 0; | ||
| size_t num_dictionary_vars = 0; | ||
| size_t all_dictionary_vars_length{0}; | ||
| size_t num_dictionary_vars{0}; | ||
| for (auto current = dictionary_var_bounds.cbegin(); dictionary_var_bounds.cend() != current;) { |
There was a problem hiding this comment.
We should assert the length is an even number, otherwise if a wrong size is given this loop will never end and segfault.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
On hold until we have resources to continue the effort. Potential action items:
|
Description
To be written
Validation performed
Verified that test cases still pass after necessary non-functional modifications.