feat(kv-ir): Improve error handling in clp::ffi::ir_stream::decoding_methods:#2089
Open
jonathan-imanu wants to merge 2 commits intoy-scope:mainfrom
Open
feat(kv-ir): Improve error handling in clp::ffi::ir_stream::decoding_methods:#2089jonathan-imanu wants to merge 2 commits intoy-scope:mainfrom
jonathan-imanu wants to merge 2 commits intoy-scope:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR refactors IR stream deserialization error handling from numeric Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/core/tests/test-ir_encoding_methods.cpp`:
- Around line 642-643: The test currently combines failure checking and specific
error matching in one REQUIRE for get_encoding_type(empty_ir_buffer,
is_four_bytes_encoding).error(); change each of these negative-path assertions
(including the instances at the other two locations) to first assert that the
result has_error() (e.g., REQUIRE(result.has_error())), then separately assert
the specific error via result.error() ==
IrDeserializationError{IrDeserializationErrorEnum::IncompleteStream}; locate the
calls to get_encoding_type and the variables empty_ir_buffer and
is_four_bytes_encoding and replace the single combined REQUIRE with the two-step
has_error() then error() checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16c021a7-c349-4361-83b6-4cf98eb4a90e
📒 Files selected for processing (12)
components/core/src/clp/clp/FileCompressor.cppcomponents/core/src/clp/ffi/ir_stream/Deserializer.hppcomponents/core/src/clp/ffi/ir_stream/IrDeserializationError.cppcomponents/core/src/clp/ffi/ir_stream/IrDeserializationError.hppcomponents/core/src/clp/ffi/ir_stream/decoding_methods.cppcomponents/core/src/clp/ffi/ir_stream/decoding_methods.hppcomponents/core/src/clp/ffi/ir_stream/ir_unit_deserialization_methods.cppcomponents/core/src/clp/ffi/ir_stream/utils.hppcomponents/core/src/clp/ir/LogEventDeserializer.cppcomponents/core/src/clp/ir/utils.cppcomponents/core/tests/test-ir_encoding_methods.cppcomponents/core/tests/test-ir_serializer.cpp
Comment on lines
+642
to
+643
| REQUIRE(get_encoding_type(empty_ir_buffer, is_four_bytes_encoding).error() | ||
| == IrDeserializationError{IrDeserializationErrorEnum::IncompleteStream}); |
Contributor
There was a problem hiding this comment.
Split these negative-path assertions into has_error() and error() checks.
These three assertions collapse “the call must fail” and “it must fail with this code” into a single check, which makes regressions harder to diagnose than the two-step pattern you already use later in this file.
Suggested update
- REQUIRE(get_encoding_type(empty_ir_buffer, is_four_bytes_encoding).error()
- == IrDeserializationError{IrDeserializationErrorEnum::IncompleteStream});
+ auto empty_result = get_encoding_type(empty_ir_buffer, is_four_bytes_encoding);
+ REQUIRE(empty_result.has_error());
+ REQUIRE(
+ empty_result.error()
+ == IrDeserializationError{IrDeserializationErrorEnum::IncompleteStream}
+ );
- REQUIRE(get_encoding_type(incomplete_buffer, is_four_bytes_encoding).error()
- == IrDeserializationError{IrDeserializationErrorEnum::IncompleteStream});
+ auto incomplete_result = get_encoding_type(incomplete_buffer, is_four_bytes_encoding);
+ REQUIRE(incomplete_result.has_error());
+ REQUIRE(
+ incomplete_result.error()
+ == IrDeserializationError{IrDeserializationErrorEnum::IncompleteStream}
+ );
- REQUIRE(get_encoding_type(invalid_ir_buffer, is_four_bytes_encoding).error()
- == IrDeserializationError{IrDeserializationErrorEnum::InvalidMagicNumber});
+ auto invalid_result = get_encoding_type(invalid_ir_buffer, is_four_bytes_encoding);
+ REQUIRE(invalid_result.has_error());
+ REQUIRE(
+ invalid_result.error()
+ == IrDeserializationError{IrDeserializationErrorEnum::InvalidMagicNumber}
+ );Also applies to: 649-650, 658-659
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/core/tests/test-ir_encoding_methods.cpp` around lines 642 - 643,
The test currently combines failure checking and specific error matching in one
REQUIRE for get_encoding_type(empty_ir_buffer, is_four_bytes_encoding).error();
change each of these negative-path assertions (including the instances at the
other two locations) to first assert that the result has_error() (e.g.,
REQUIRE(result.has_error())), then separately assert the specific error via
result.error() ==
IrDeserializationError{IrDeserializationErrorEnum::IncompleteStream}; locate the
calls to get_encoding_type and the variables empty_ir_buffer and
is_four_bytes_encoding and replace the single combined REQUIRE with the two-step
has_error() then error() checks.
LinZhihao-723
requested changes
Mar 14, 2026
Member
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Will stop reading as I realized many API changes in this PR don't make sense as explained in the comment.
And in fact, I realized most of these APIs aren't needed anymore once we support using the kv-ir deserializer to deserialize the old IR format.
That said, I think the right way to do is probably:
- Leave those APIs not used by kv-ir deserializer untouched in this PR, and only add result-style versions for those used by kv-ir deserializer (for example,
deserialize_int). - Work on supporting the old IR format in the kv-ir deserializer next.
- Use kv-ir deserializer to implement
ir::LogEventDeserializer. - Move the rest unused APIs to a new namespace for deprecation.
Let me know ur thoughts.
Comment on lines
+62
to
+63
| auto get_encoding_type(ReaderInterface& reader, bool& is_four_bytes_encoding) | ||
| -> ystdlib::error_handling::Result<void>; |
Member
There was a problem hiding this comment.
This would be a bad practice we should try to avoid: if we're using result-based error handling, then we should use the result to return is_four_bytes_encoding.
- The easy fix is to make the return type
ystdlib::error_handling::Result<bool> - However, it's still a bit hacky and personally I'd create an enum named
EncodingTypewhich contains the encoding type we support.
Depends on u whether u want to keep this PR to step 1, or go further to step 2.
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
2 participants
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.
Description
This PR is the second step in improving the error handling in
clp::ffi::ir_stream::Deserializer.hpp. Following the suggestion of #904, the end goal is to use theResult<T, IrErrorCode>as the return type instead of usingstd::errc. This PR builds off of the work done in #2005.Since this is a larger refactor, this step focuses changing functions in
components/core/src/clp/ffi/ir_stream/decoding_methods.cppto returnResult<T, IrErrorCode>instead of the C-styleIRErrorCode.Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Bug Fixes
Refactor
Tests