refactor(kv-ir): Improve error handling in clp::ffi::ir_stream::Serializer.#2000
Open
jonathan-imanu wants to merge 7 commits intoy-scope:mainfrom
Open
refactor(kv-ir): Improve error handling in clp::ffi::ir_stream::Serializer.#2000jonathan-imanu wants to merge 7 commits intoy-scope:mainfrom
clp::ffi::ir_stream::Serializer.#2000jonathan-imanu wants to merge 7 commits intoy-scope:mainfrom
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdded a new IrSerializationError enum/type and error-category implementation, refactored IR serialization APIs and helpers to return typed ystdlib::error_handling::Result<void, IrSerializationError> (and adjusted related call sites/tests), and adjusted IrErrorCode message text casing and punctuation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/core/src/clp/ffi/ir_stream/Serializer.cpp (1)
592-713:⚠️ Potential issue | 🟠 MajorPreserve schema‑tree node errors during map serialization.
Failures fromserialize_schema_tree_nodeare collapsed intoKeyValuePairSerializationFailure, soSchemaTreeNodeSerializationFailureis never surfaced. Capture and return the underlying error when DFS fails; this also avoids the negatedhas_error()checks.As per coding guidelines, prefer `false == ` rather than `!`.💡 Proposed fix to preserve schema‑tree node errors
+ std::optional<IrErrorCode> schema_tree_error; auto auto_gen_schema_tree_node_serialization_method - = [this](SchemaTree::NodeLocator const& locator) -> bool { - return !this->serialize_schema_tree_node<true>(locator).has_error(); - }; + = [this, &schema_tree_error](SchemaTree::NodeLocator const& locator) -> bool { + auto const result = this->serialize_schema_tree_node<true>(locator); + if (result.has_error()) { + schema_tree_error = result.error(); + return false; + } + return true; + }; @@ - if (0 != auto_gen_kv_pairs_map.size - && false - == serialize_msgpack_map_using_dfs( - auto_gen_kv_pairs_map, - m_auto_gen_keys_schema_tree, - auto_gen_schema_tree_node_serialization_method, - auto_gen_node_id_value_pairs_serialization_method, - auto_gen_empty_map_serialization_method - )) - { - return IrErrorCode{IrErrorCodeEnum::KeyValuePairSerializationFailure}; - } + if (0 != auto_gen_kv_pairs_map.size + && false + == serialize_msgpack_map_using_dfs( + auto_gen_kv_pairs_map, + m_auto_gen_keys_schema_tree, + auto_gen_schema_tree_node_serialization_method, + auto_gen_node_id_value_pairs_serialization_method, + auto_gen_empty_map_serialization_method + )) + { + if (schema_tree_error.has_value()) { + return schema_tree_error.value(); + } + return IrErrorCode{IrErrorCodeEnum::KeyValuePairSerializationFailure}; + } + + schema_tree_error.reset(); auto user_gen_schema_tree_node_serialization_method - = [this](SchemaTree::NodeLocator const& locator) -> bool { - return !this->serialize_schema_tree_node<false>(locator).has_error(); - }; + = [this, &schema_tree_error](SchemaTree::NodeLocator const& locator) -> bool { + auto const result = this->serialize_schema_tree_node<false>(locator); + if (result.has_error()) { + schema_tree_error = result.error(); + return false; + } + return true; + }; @@ - if (false - == serialize_msgpack_map_using_dfs( - user_gen_kv_pairs_map, - m_user_gen_keys_schema_tree, - user_gen_schema_tree_node_serialization_method, - user_gen_node_id_value_pairs_serialization_method, - user_gen_empty_map_serialization_method - )) - { - return IrErrorCode{IrErrorCodeEnum::KeyValuePairSerializationFailure}; - } + if (false + == serialize_msgpack_map_using_dfs( + user_gen_kv_pairs_map, + m_user_gen_keys_schema_tree, + user_gen_schema_tree_node_serialization_method, + user_gen_node_id_value_pairs_serialization_method, + user_gen_empty_map_serialization_method + )) + { + if (schema_tree_error.has_value()) { + return schema_tree_error.value(); + } + return IrErrorCode{IrErrorCodeEnum::KeyValuePairSerializationFailure}; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp/ffi/ir_stream/Serializer.cpp` around lines 592 - 713, The map-DFS currently collapses errors from serialize_schema_tree_node into a generic KeyValuePairSerializationFailure; change auto_gen_schema_tree_node_serialization_method and user_gen_schema_tree_node_serialization_method to return and propagate the actual IrErrorCode (or Result) from serialize_schema_tree_node instead of returning bool via !has_error(), then update serialize_msgpack_map_using_dfs call sites to capture and return that underlying error when it fails (return the SchemaTreeNodeSerializationFailure when appropriate) rather than always returning IrErrorCode{IrErrorCodeEnum::KeyValuePairSerializationFailure}; also replace occurrences of !expr with the preferred style (false == expr) as per guidelines.components/core/src/clp_s/log_converter/LogSerializer.hpp (1)
58-69:⚠️ Potential issue | 🟡 MinorUpdate the no‑timestamp overload docs to match the new error code.
The overload below still referencesstd::errc::invalid_argument, but failures now propagateIrErrorCodeEnum::KeyValuePairSerializationFailure.✏️ Suggested doc fix
- * - std::errc::invalid_argument if `clp::ffi::ir_stream::Serializer<>::serialize_msgpack_map` - * returns on failure. + * - IrErrorCodeEnum::KeyValuePairSerializationFailure if + * `clp::ffi::ir_stream::Serializer<>::serialize_msgpack_map` fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/core/src/clp_s/log_converter/LogSerializer.hpp` around lines 58 - 69, Update the doc comment for the no-timestamp overload of add_message (the overload declared as add_message(std::string_view message)) to state that failures now return IrErrorCodeEnum::KeyValuePairSerializationFailure instead of std::errc::invalid_argument; edit the bullet under `@return` to replace the old enum with IrErrorCodeEnum::KeyValuePairSerializationFailure and keep the description that it originates from clp::ffi::ir_stream::Serializer<>::serialize_msgpack_map failures.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@components/core/src/clp_s/log_converter/LogSerializer.hpp`:
- Around line 58-69: Update the doc comment for the no-timestamp overload of
add_message (the overload declared as add_message(std::string_view message)) to
state that failures now return IrErrorCodeEnum::KeyValuePairSerializationFailure
instead of std::errc::invalid_argument; edit the bullet under `@return` to replace
the old enum with IrErrorCodeEnum::KeyValuePairSerializationFailure and keep the
description that it originates from
clp::ffi::ir_stream::Serializer<>::serialize_msgpack_map failures.
In `@components/core/src/clp/ffi/ir_stream/Serializer.cpp`:
- Around line 592-713: The map-DFS currently collapses errors from
serialize_schema_tree_node into a generic KeyValuePairSerializationFailure;
change auto_gen_schema_tree_node_serialization_method and
user_gen_schema_tree_node_serialization_method to return and propagate the
actual IrErrorCode (or Result) from serialize_schema_tree_node instead of
returning bool via !has_error(), then update serialize_msgpack_map_using_dfs
call sites to capture and return that underlying error when it fails (return the
SchemaTreeNodeSerializationFailure when appropriate) rather than always
returning IrErrorCode{IrErrorCodeEnum::KeyValuePairSerializationFailure}; also
replace occurrences of !expr with the preferred style (false == expr) as per
guidelines.
In `@components/core/tests/test-ir_encoding_methods.cpp`:
- Around line 411-419: The conditional checks using negation should be rewritten
to the repository style using explicit equality to false: replace occurrences
like "!serializer.serialize_msgpack_map(...).has_error()" with "false ==
serializer.serialize_msgpack_map(...).has_error()" for the calls to
serializer.serialize_msgpack_map involving msgpack_obj.via.map and
msgpack_empty_map_obj.via.map so both branches use the `false == <expression>`
style when testing has_error().
Comment on lines
411
to
419
| if (!serializer.serialize_msgpack_map(msgpack_obj.via.map, msgpack_empty_map_obj.via.map) | ||
| .has_error()) | ||
| { | ||
| // Serialization should fail | ||
| return false; | ||
| } | ||
| if (serializer.serialize_msgpack_map(msgpack_empty_map_obj.via.map, msgpack_obj.via.map)) { | ||
| if (!serializer.serialize_msgpack_map(msgpack_empty_map_obj.via.map, msgpack_obj.via.map) | ||
| .has_error()) | ||
| { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the specific lines in the file
cat -n components/core/tests/test-ir_encoding_methods.cpp | sed -n '405,425p'Repository: y-scope/clp
Length of output: 989
🏁 Script executed:
# Search for coding style guidelines in the repository
fd -t f -e md -e txt -e rst | xargs grep -l "false ==" | head -20Repository: y-scope/clp
Length of output: 37
🏁 Script executed:
# Check if there's a STYLE, CODING_GUIDELINES, or similar documentation file
fd -t f \( -name "*STYLE*" -o -name "*GUIDE*" -o -name "*CONVENTION*" \) | head -20Repository: y-scope/clp
Length of output: 285
🏁 Script executed:
# Search for how error checks are typically done in the test file
grep -n "\.has_error()" components/core/tests/test-ir_encoding_methods.cpp | head -20Repository: y-scope/clp
Length of output: 878
🏁 Script executed:
# Check the broader context around lines 411-419
cat -n components/core/tests/test-ir_encoding_methods.cpp | sed -n '400,430p'Repository: y-scope/clp
Length of output: 1328
🏁 Script executed:
# Let's check if there's a CODING_GUIDELINES or similar document to confirm this style rule
find . -type f \( -name "*.md" -o -name "*.txt" -o -name ".clang-format" -o -name "CONTRIBUTING*" \) | head -20Repository: y-scope/clp
Length of output: 911
🏁 Script executed:
# Let's also check the clang-format configuration if it exists
find . -name ".clang-format" -o -name "clang-format" | xargs cat 2>/dev/null | head -50Repository: y-scope/clp
Length of output: 761
Use false == style instead of ! for error checks.
Align these conditionals with the repository's coding style guideline that prefers false == <expression> rather than !<expression>.
Suggested change
- if (!serializer.serialize_msgpack_map(msgpack_obj.via.map, msgpack_empty_map_obj.via.map)
- .has_error())
+ if (false
+ == serializer.serialize_msgpack_map(msgpack_obj.via.map, msgpack_empty_map_obj.via.map)
+ .has_error())
{
// Serialization should fail
return false;
}
- if (!serializer.serialize_msgpack_map(msgpack_empty_map_obj.via.map, msgpack_obj.via.map)
- .has_error())
+ if (false
+ == serializer.serialize_msgpack_map(msgpack_empty_map_obj.via.map, msgpack_obj.via.map)
+ .has_error())
{
// Serialization should fail
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!serializer.serialize_msgpack_map(msgpack_obj.via.map, msgpack_empty_map_obj.via.map) | |
| .has_error()) | |
| { | |
| // Serialization should fail | |
| return false; | |
| } | |
| if (serializer.serialize_msgpack_map(msgpack_empty_map_obj.via.map, msgpack_obj.via.map)) { | |
| if (!serializer.serialize_msgpack_map(msgpack_empty_map_obj.via.map, msgpack_obj.via.map) | |
| .has_error()) | |
| { | |
| if (false | |
| == serializer.serialize_msgpack_map(msgpack_obj.via.map, msgpack_empty_map_obj.via.map) | |
| .has_error()) | |
| { | |
| // Serialization should fail | |
| return false; | |
| } | |
| if (false | |
| == serializer.serialize_msgpack_map(msgpack_empty_map_obj.via.map, msgpack_obj.via.map) | |
| .has_error()) | |
| { | |
| // Serialization should fail | |
| return false; | |
| } |
🤖 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 411 - 419,
The conditional checks using negation should be rewritten to the repository
style using explicit equality to false: replace occurrences like
"!serializer.serialize_msgpack_map(...).has_error()" with "false ==
serializer.serialize_msgpack_map(...).has_error()" for the calls to
serializer.serialize_msgpack_map involving msgpack_obj.via.map and
msgpack_empty_map_obj.via.map so both branches use the `false == <expression>`
style when testing has_error().
jonathan-imanu
changed the title
clp::ffi::ir_stream::Serializer (addresses #904).clp::ffi::ir_stream::Serializer.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/src/clp/ffi/ir_stream/IrErrorCode.cpp`:
- Around line 25-37: Reorder the switch cases in IrErrorCode.cpp to match the
enum declaration order in IrErrorCode.hpp: MetadataSerializationFailure,
SchemaTreeNodeSerializationFailure, KeyValuePairSerializationFailure,
UnsupportedUserDefinedMetadata, UnsupportedSchemaTreeNodeType,
SchemaTreeNodeIdSerializationFailure; update the case blocks for IrErrorCodeEnum
to that sequence so reviewers can easily verify coverage and maintain parity
with the IrErrorCodeEnum declaration.
In `@components/core/src/clp/ffi/ir_stream/Serializer.cpp`:
- Around line 598-627: Inline the temporary encode_result when calling
YSTDLIB_ERROR_HANDLING_TRYV to remove the redundant variable: replace the
pattern where encode_result =
encode_and_serialize_schema_tree_node_id<...>(node_id,
m_sequential_serialization_buf); YSTDLIB_ERROR_HANDLING_TRYV(encode_result);
with a direct call
YSTDLIB_ERROR_HANDLING_TRYV(encode_and_serialize_schema_tree_node_id<...>(node_id,
m_sequential_serialization_buf)); do this for every lambda that calls
encode_and_serialize_schema_tree_node_id (e.g., the auto-generated value-pair
lambda that precedes serialize_value and
auto_gen_empty_map_serialization_method) so the code calls
YSTDLIB_ERROR_HANDLING_TRYV(...) inline and otherwise leaves serialize_value,
serialize_value_empty_object, and m_sequential_serialization_buf usage
unchanged.
Comment on lines
25
to
37
| case IrErrorCodeEnum::KeyValuePairSerializationFailure: | ||
| return "Failed to serialize a key-value pair log event."; | ||
| case IrErrorCodeEnum::MetadataSerializationFailure: | ||
| return "Failed to serialize the stream's metadata."; | ||
| case IrErrorCodeEnum::SchemaTreeNodeSerializationFailure: | ||
| return "Failed to serialize a schema tree node."; | ||
| case IrErrorCodeEnum::SchemaTreeNodeIdSerializationFailure: | ||
| return "Failed to serialize a schema tree node ID. The ID exceeds the representable " | ||
| "range."; | ||
| case IrErrorCodeEnum::UnsupportedSchemaTreeNodeType: | ||
| return "The schema tree node type is not supported."; | ||
| case IrErrorCodeEnum::UnsupportedUserDefinedMetadata: | ||
| return "The user-defined metadata is not a valid JSON object."; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Nit: Switch case ordering doesn't match the enum declaration order.
In IrErrorCode.hpp, the enum members are declared in the order: MetadataSerializationFailure, SchemaTreeNodeSerializationFailure, KeyValuePairSerializationFailure, UnsupportedUserDefinedMetadata, UnsupportedSchemaTreeNodeType, SchemaTreeNodeIdSerializationFailure. Here the switch cases use a different order. Matching the declaration order makes it easier to verify that all cases are covered.
🔧 Suggested reordering to match the enum declaration
- case IrErrorCodeEnum::KeyValuePairSerializationFailure:
- return "Failed to serialize a key-value pair log event.";
case IrErrorCodeEnum::MetadataSerializationFailure:
return "Failed to serialize the stream's metadata.";
case IrErrorCodeEnum::SchemaTreeNodeSerializationFailure:
return "Failed to serialize a schema tree node.";
+ case IrErrorCodeEnum::KeyValuePairSerializationFailure:
+ return "Failed to serialize a key-value pair log event.";
+ case IrErrorCodeEnum::UnsupportedUserDefinedMetadata:
+ return "The user-defined metadata is not a valid JSON object.";
+ case IrErrorCodeEnum::UnsupportedSchemaTreeNodeType:
+ return "The schema tree node type is not supported.";
case IrErrorCodeEnum::SchemaTreeNodeIdSerializationFailure:
return "Failed to serialize a schema tree node ID. The ID exceeds the representable "
"range.";
- case IrErrorCodeEnum::UnsupportedSchemaTreeNodeType:
- return "The schema tree node type is not supported.";
- case IrErrorCodeEnum::UnsupportedUserDefinedMetadata:
- return "The user-defined metadata is not a valid JSON object.";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case IrErrorCodeEnum::KeyValuePairSerializationFailure: | |
| return "Failed to serialize a key-value pair log event."; | |
| case IrErrorCodeEnum::MetadataSerializationFailure: | |
| return "Failed to serialize the stream's metadata."; | |
| case IrErrorCodeEnum::SchemaTreeNodeSerializationFailure: | |
| return "Failed to serialize a schema tree node."; | |
| case IrErrorCodeEnum::SchemaTreeNodeIdSerializationFailure: | |
| return "Failed to serialize a schema tree node ID. The ID exceeds the representable " | |
| "range."; | |
| case IrErrorCodeEnum::UnsupportedSchemaTreeNodeType: | |
| return "The schema tree node type is not supported."; | |
| case IrErrorCodeEnum::UnsupportedUserDefinedMetadata: | |
| return "The user-defined metadata is not a valid JSON object."; | |
| case IrErrorCodeEnum::MetadataSerializationFailure: | |
| return "Failed to serialize the stream's metadata."; | |
| case IrErrorCodeEnum::SchemaTreeNodeSerializationFailure: | |
| return "Failed to serialize a schema tree node."; | |
| case IrErrorCodeEnum::KeyValuePairSerializationFailure: | |
| return "Failed to serialize a key-value pair log event."; | |
| case IrErrorCodeEnum::UnsupportedUserDefinedMetadata: | |
| return "The user-defined metadata is not a valid JSON object."; | |
| case IrErrorCodeEnum::UnsupportedSchemaTreeNodeType: | |
| return "The schema tree node type is not supported."; | |
| case IrErrorCodeEnum::SchemaTreeNodeIdSerializationFailure: | |
| return "Failed to serialize a schema tree node ID. The ID exceeds the representable " | |
| "range."; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/core/src/clp/ffi/ir_stream/IrErrorCode.cpp` around lines 25 - 37,
Reorder the switch cases in IrErrorCode.cpp to match the enum declaration order
in IrErrorCode.hpp: MetadataSerializationFailure,
SchemaTreeNodeSerializationFailure, KeyValuePairSerializationFailure,
UnsupportedUserDefinedMetadata, UnsupportedSchemaTreeNodeType,
SchemaTreeNodeIdSerializationFailure; update the case blocks for IrErrorCodeEnum
to that sequence so reviewers can easily verify coverage and maintain parity
with the IrErrorCodeEnum declaration.
LinZhihao-723
requested changes
Feb 19, 2026
Member
LinZhihao-723
left a comment
LinZhihao-723
left a comment
There was a problem hiding this comment.
Some high-level comments to fix.
I will review the docstring changes in detail in the next round.
Member
There was a problem hiding this comment.
Since IrErrorCode is std::errc compatible, I think returning a ystlib::error_handling::Result<T> should be better. This allows us to call methods that also return a std::errc on error (which can come from another library that doesn't use IrErrorCode).
Member
There was a problem hiding this comment.
By looking at this, I think it might be worth distinguishing the serialization and deserialization errors.
How about:
- For this PR, let's introduce
IrSerializationErrorEnumandIrSerializationError, and leaveIrErrorCodeEnumandIrErrorCodeuntouched. - In the deserialization PR, let's rename
IrErrorCodeEnumandIrErrorCodetoIrDeserializationErrorEnumandIrDeserializationError.
Comment on lines
25
to
37
| case IrErrorCodeEnum::KeyValuePairSerializationFailure: | ||
| return "Failed to serialize a key-value pair log event."; | ||
| case IrErrorCodeEnum::MetadataSerializationFailure: | ||
| return "Failed to serialize the stream's metadata."; | ||
| case IrErrorCodeEnum::SchemaTreeNodeSerializationFailure: | ||
| return "Failed to serialize a schema tree node."; | ||
| case IrErrorCodeEnum::SchemaTreeNodeIdSerializationFailure: | ||
| return "Failed to serialize a schema tree node ID. The ID exceeds the representable " | ||
| "range."; | ||
| case IrErrorCodeEnum::UnsupportedSchemaTreeNodeType: | ||
| return "The schema tree node type is not supported."; | ||
| case IrErrorCodeEnum::UnsupportedUserDefinedMetadata: | ||
| return "The user-defined metadata is not a valid JSON object."; |
Member
There was a problem hiding this comment.
The existing error messages are a bit outdated. We should follow the latest rule which:
- Don't capitalize the first character
- Don't add a period at the end
This is because normally, this error message is added to a format string and printed in the log. The above changes would make this formatted string easier to manage.
|
|
||
| namespace clp::ffi::ir_stream { | ||
| namespace { | ||
| using SerializationResult = ystdlib::error_handling::Result<void, IrErrorCode>; |
Member
There was a problem hiding this comment.
Same, I think we don't need this alias. Using the default error type std::errc should be sufficient.
…p API return types
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/core/src/clp/ffi/ir_stream/Serializer.cpp`:
- Around line 614-620: Multiple places create a redundant local variable
encode_result immediately passed to the YSTDLIB_ERROR_HANDLING_TRYV macro;
remove the intermediate variable and invoke the macro directly with the call to
encode_and_serialize_schema_tree_node_id<...>(...) instead. Update all
occurrences (the four lambdas inside serialize_msgpack_map and the
serialize_schema_tree_node function) to replace patterns like auto encode_result
= encode_and_serialize_schema_tree_node_id<...>(...);
YSTDLIB_ERROR_HANDLING_TRYV(encode_result); with a single
YSTDLIB_ERROR_HANDLING_TRYV(encode_and_serialize_schema_tree_node_id<...>(...));
keeping the same template parameters, node_id argument, and
m_sequential_serialization_buf usage so behavior is unchanged.
In `@components/core/tests/test-ir_encoding_methods.cpp`:
- Around line 412-425: The test uses "true == ... .has_value()" which is
inconsistent with the repo's established pattern; change both checks that call
serializer.serialize_msgpack_map(...) to instead assert the negative has_error
pattern (use "false == <result>.has_error()") so the positive success assertion
matches other tests — locate the calls to serialize_msgpack_map with
msgpack_obj.via.map and msgpack_empty_map_obj.via.map and replace the
has_value() style with false == <result>.has_error() for consistency.
LinZhihao-723
requested changes
Feb 20, 2026
Comment on lines
+105
to
+107
| * - Forwards `Serializer::serialize_msgpack_map`'s return values. | ||
| * - IrSerializationErrorEnum::KeyValuePairSerializationFailure if the msgpack bytes don't | ||
| * represent maps. |
Member
There was a problem hiding this comment.
Suggested change
| * - Forwards `Serializer::serialize_msgpack_map`'s return values. | |
| * - IrSerializationErrorEnum::KeyValuePairSerializationFailure if the msgpack bytes don't | |
| * represent maps. | |
| * - IrSerializationErrorEnum::KeyValuePairSerializationFailure if the msgpack bytes cannot be | |
| * deserialized into a map. | |
| * - Forwards `Serializer::serialize_msgpack_map`'s return values. |
A little rewording.
Also, we prefer to list returned enums before forwarded errors.
Comment on lines
113
to
+118
| REQUIRE(unpack_and_serialize_msgpack_bytes( | ||
| nlohmann::json::to_msgpack(auto_gen_kv_pairs), | ||
| nlohmann::json::to_msgpack(user_gen_kv_pairs), | ||
| serializer | ||
| )); | ||
| nlohmann::json::to_msgpack(auto_gen_kv_pairs), | ||
| nlohmann::json::to_msgpack(user_gen_kv_pairs), | ||
| serializer | ||
| ) | ||
| .has_value()); |
Member
There was a problem hiding this comment.
REQUIRE_FALSE(unpack_and_serialize_msgpack_bytes(
nlohmann::json::to_msgpack(auto_gen_kv_pairs),
nlohmann::json::to_msgpack(user_gen_kv_pairs),
serializer
)
.has_error());We prefer to use .has_error to check if the result contains an error.
| /** | ||
| * This enum class represents all possible error codes related to serializing or deserializing CLP | ||
| * IR streams. | ||
| * This enum class represents error codes for IR stream deserialization |
Member
There was a problem hiding this comment.
Suggested change
| * This enum class represents error codes for IR stream deserialization | |
| * Error code enum for IR stream deserialization. |
|
|
||
| namespace clp::ffi::ir_stream { | ||
| /** | ||
| * This enum class represents error codes for IR stream serialization |
Member
There was a problem hiding this comment.
Suggested change
| * This enum class represents error codes for IR stream serialization | |
| * Error code enum for IR stream serialization. |
Comment on lines
+27
to
+28
| return "failed to serialize a schema tree node ID (the ID exceeds the representable " | ||
| "range)"; |
Member
There was a problem hiding this comment.
Suggested change
| return "failed to serialize a schema tree node ID (the ID exceeds the representable " | |
| "range)"; | |
| return "failed to serialize a schema tree node ID"; |
Since we didn't include details in other error enums, I think it's fair to keep it simple.
| MetadataSerializationFailure, | ||
| SchemaTreeNodeSerializationFailure, | ||
| SchemaTreeNodeIdSerializationFailure, | ||
| UnsupportedSchemaTreeNodeType, |
Member
There was a problem hiding this comment.
How about UnsupportedSchemaTreeNodeType -> UnknownSchemaTreeNodeType (and also need to update the error message))?
Comment on lines
+119
to
+123
| * - IrSerializationErrorEnum::UnsupportedSchemaTreeNodeType if the node type is unsupported. | ||
| * - IrSerializationErrorEnum::SchemaTreeNodeIdSerializationFailure if the parent node ID | ||
| * couldn't be serialized. | ||
| * - IrSerializationErrorEnum::SchemaTreeNodeSerializationFailure if the key name couldn't be | ||
| * serialized. |
Member
There was a problem hiding this comment.
Suggested change
| * - IrSerializationErrorEnum::UnsupportedSchemaTreeNodeType if the node type is unsupported. | |
| * - IrSerializationErrorEnum::SchemaTreeNodeIdSerializationFailure if the parent node ID | |
| * couldn't be serialized. | |
| * - IrSerializationErrorEnum::SchemaTreeNodeSerializationFailure if the key name couldn't be | |
| * serialized. | |
| * - IrSerializationErrorEnum::UnsupportedSchemaTreeNodeType if the node type is unsupported. | |
| * - IrSerializationErrorEnum::SchemaTreeNodeSerializationFailure if the key name couldn't be | |
| * serialized. | |
| * - Forwards `encode_and_serialize_schema_tree_node_id`'s return value on failure. |
Comment on lines
+88
to
+90
| REQUIRE( | ||
| serializer.serialize_msgpack_map(auto_gen_obj.via.map, user_gen_obj.via.map).has_value() | ||
| ); |
Member
There was a problem hiding this comment.
Suggested change
| REQUIRE( | |
| serializer.serialize_msgpack_map(auto_gen_obj.via.map, user_gen_obj.via.map).has_value() | |
| ); | |
| REQUIRE_FALSE( | |
| serializer.serialize_msgpack_map(auto_gen_obj.via.map, user_gen_obj.via.map).has_error() | |
| ); |
Comment on lines
+412
to
+414
| if (true | ||
| == serializer.serialize_msgpack_map(msgpack_obj.via.map, msgpack_empty_map_obj.via.map) | ||
| .has_value()) |
Member
There was a problem hiding this comment.
Suggested change
| if (true | |
| == serializer.serialize_msgpack_map(msgpack_obj.via.map, msgpack_empty_map_obj.via.map) | |
| .has_value()) | |
| if (false | |
| == serializer.serialize_msgpack_map(msgpack_obj.via.map, msgpack_empty_map_obj.via.map) | |
| .has_error()) |
Btw we don't need true == condition; we only use false == xxx for Yoda condition.
Comment on lines
+1517
to
+1520
| REQUIRE( | ||
| (clp::ffi::ir_stream::IrSerializationErrorEnum::UnsupportedUserDefinedMetadata | ||
| == serializer_result.error().get_error()) | ||
| ); |
Member
There was a problem hiding this comment.
Suggested change
| REQUIRE( | |
| (clp::ffi::ir_stream::IrSerializationErrorEnum::UnsupportedUserDefinedMetadata | |
| == serializer_result.error().get_error()) | |
| ); | |
| using clp::ffi::ir_stream::IrSerializationError; | |
| using clp::ffi::ir_stream::IrSerializationErrorEnum; | |
| REQUIRE(IrSerializationError{IrSerializationErrorEnum::UnsupportedUserDefinedMetadata} | |
| == serializer_result.error()); |
The error enum itself is not strong-typed. Need to check the error object by constructing an IrSerializationError.
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 refactors the
clp::ffi::ir_stream::SerializerAPI to replace theboolandstd::errcreturn types with typedResult<T, IrErrorCode>. This provides more descriptive error reporting for IR stream serialization failures. It updates tests accordingly. It is the first step in addressing #904Checklist
breaking change.
Validation performed
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests