feat(ffi): Add initial implementation of IrErrorCode (using the ErrorCode template) which will replace the IRErrorCode enum.
#623
Conversation
WalkthroughThe changes in this pull request involve the addition of new error handling functionality within the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
components/core/src/clp/ffi/ir_stream/IrErrorCode.hpp (2)
9-11: Complete the documentation commentThe documentation comment is incomplete and should describe the purpose of the
IrErrorCodeEnum./** - * Enums that define all + * Enums that define all possible error conditions that can occur during IR stream + * serialization and deserialization operations. */
12-16: Document individual error codesEach error code should be documented to explain when it occurs and how it should be handled.
enum class IrErrorCodeEnum : uint8_t { + /// Indicates a failure in the decoding method implementation DecodingMethodFailure, + /// Indicates that the end of the IR stream has been reached EndOfStream, + /// Indicates that the IR stream is incomplete or truncated IncompleteStream, };components/core/src/clp/ffi/ir_stream/IrErrorCode.cpp (1)
14-25: Enhance error messages with more contextThe error messages could be more descriptive to help with debugging and error handling.
switch (error_enum) { case IrErrorCodeEnum::DecodingMethodFailure: - return "Decoding method failure."; + return "IR stream decoding method failed to process the input data."; case IrErrorCodeEnum::EndOfStream: - return "End-of-stream has been reached."; + return "Reached the end of IR stream while more data was expected."; case IrErrorCodeEnum::IncompleteStream: - return "Incomplete IR stream."; + return "IR stream is incomplete or truncated, preventing complete deserialization."; default: return "Unknown error code enum."; }components/core/tests/test-error_handling.cpp (1)
12-12: Consider organizing includesThe new include for
IrErrorCode.hppshould be grouped with other project includes.-#include "../src/clp/ffi/ir_stream/IrErrorCode.hpp" + +// Project includes #include "../src/clp/error_handling/ErrorCode.hpp" +#include "../src/clp/ffi/ir_stream/IrErrorCode.hpp"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
components/core/CMakeLists.txt(1 hunks)components/core/src/clp/ffi/ir_stream/IrErrorCode.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/IrErrorCode.hpp(1 hunks)components/core/tests/test-error_handling.cpp(2 hunks)
🧰 Additional context used
📓 Learnings (2)
components/core/CMakeLists.txt (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#549
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:209-210
Timestamp: 2024-11-10T16:46:53.300Z
Learning: Until #486 is merged with customized error code support, introducing a layer from `IRErrorCode` to `std::errc` is unnecessary.
components/core/src/clp/ffi/ir_stream/IrErrorCode.hpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#549
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:209-210
Timestamp: 2024-11-10T16:46:53.300Z
Learning: Until #486 is merged with customized error code support, introducing a layer from `IRErrorCode` to `std::errc` is unnecessary.
🔇 Additional comments (1)
components/core/CMakeLists.txt (1)
385-386: LGTM! Verify dependency on PR #486.
The addition of IrErrorCode files to the unit test sources is correctly placed and aligns with the PR objectives for implementing the new error-handling system.
Let's verify the status of PR #486 which this change depends on:
✅ Verification successful
PR #486 is already merged, changes are properly sequenced
The dependency on PR #486 for error code support has been satisfied, as it was merged on December 2nd, 2024. The addition of IrErrorCode files builds upon the ErrorCode template infrastructure introduced in PR #486, maintaining the correct sequencing of changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if PR #486 has been merged
# Expected result: PR #486 should be merged before this PR
gh pr view 486 --json state,mergedAt,title
Length of output: 225
| case IrErrorCodeEnum::DecodingMethodFailure: | ||
| return "Decoding method failure."; | ||
| case IrErrorCodeEnum::EndOfStream: | ||
| return "End-of-stream has been reached."; | ||
| case IrErrorCodeEnum::IncompleteStream: | ||
| return "Incomplete IR stream."; |
There was a problem hiding this comment.
Lets try to be a bit more descriptive for these messages, since right now they just repeat the enum name for the most part. Ideally, we want the user to have some idea on where to look to understand the source of the error.
For example, for EndOfStream maybe:
"The end-of-stream encoding tag has been consumed."
I'm not sure that is very good, but I think if we discuss for a bit we can come up with some better stuff.
LinZhihao-723
changed the title
IrErrorCode.IrErrorCode to replace the IRErrorCode enum using the ErrorCode template.
Co-authored-by: davidlion <[email protected]>
|
Updated PR title according to offline discussion |
davidlion
changed the title
IrErrorCode to replace the IRErrorCode enum using the ErrorCode template.IrErrorCode (using the ErrorCode template) which will replace the IRErrorCode enum.
…rorCode` template) which will replace the `IRErrorCode` enum. (y-scope#623)
Description
The existing error handling of CLP IR serialization/deserialization relies on the enum
IRErrorCode, defined indecoding_methods.hpp. However, this enum has many limitations and should be deprecated by the new error-handling system introduced in PR #486.This PR introduces a boilerplate for the new IR error code,
IrErrorCode,based on the new error-handling system from #486. So far, we have introduced only three error code enums that are general enough for existing IR errors. More error enums will be added in future PRs whenIrErrorCodeis used inside the current serialization/deserialization code path.Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Tests