feat(core): Add ErrorCode template to standardize conversion of user-defined error code enums to std::error_code.
#486
Conversation
There was a problem hiding this comment.
Lets resolve some of the inconsistent naming:
- we switch between
erranderrorthroughout the code errnoinget_errnoanderror_numin other places should be the same as far as I can tellm_erroris a little ambiguous given almost everything is prefixed with "error" and its getter (currentlyget_err_enum) should match the name
I think it would be good if we add a test where we make a condition with a stdlib error_code and an error_code from your template to sanity check / ensure we don't break any functionality.
|
Made some of the methods inline to the template declaration so that they can't be specialized |
There was a problem hiding this comment.
The major thing I'm concerned with is how to actually use this error-handling library beyond the examples provided in the unit tests.
From what I've seen after testing out this library myself:
- The templates are implemented in the namespace
clp::error_handling, so the overridden function implementations need to be in the same namespace as well. This is a bit counter-intuitive if you are working in a different namespace. - Ideally the ErrorCode enum class and the actual ErrorCode class should be placed in another header file so that the same error code system can be potentially used across different files. They could also be declared in a namespace that' could be entirely orthogonal to
clp::error_handling. I don't have a clear clue on how that can be achieved. In the unit tests, these classes are defined in a global scope, and not in a header file. So a README that can guide library users would be very helpful. - Once the error code library works 100%, we can implement the same for the error conditions, and then figure out all possible scenarios of them comparing against each other.
- we've discussed things like using small/harmless macros to help library users implementing their custom error codes. But this is not as important as figuring out how to use it properly first.
|
WalkthroughThe changes introduced in this pull request involve the addition of a new error handling subsystem within the CLP project. A new header file, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ErrorHandling
participant TestSuite
User->>ErrorHandling: Trigger error handling
ErrorHandling->>ErrorHandling: Create ErrorCode instance
ErrorHandling->>ErrorHandling: Retrieve error category
ErrorHandling->>ErrorHandling: Get error message
ErrorHandling-->>User: Return error message
User->>TestSuite: Run tests
TestSuite->>ErrorHandling: Validate error codes
TestSuite-->>User: Provide test results
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 while we investigate modern ways of writing CMake scripts for libraries. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
components/core/src/clp/error_handling/CMakeLists.txt (1)
1-4: Consider using full path for header fileThe header list uses a relative path which might cause issues if the CMake file is included from different contexts.
set( ERROR_HANDLING_HEADER_LIST - "ErrorCode.hpp" + "${CMAKE_CURRENT_SOURCE_DIR}/ErrorCode.hpp" )components/core/CMakeLists.txt (1)
212-214: Consider grouping core utility modules together.The error handling, regex utils, and string utils appear to be core utility modules. Consider grouping them more explicitly, either through CMake variables or directory structure, to better reflect their shared purpose as foundational utilities.
Example approach:
-add_subdirectory(src/clp/error_handling) -add_subdirectory(src/clp/regex_utils) -add_subdirectory(src/clp/string_utils) +# Core utility modules +set(CORE_UTILITY_MODULES + src/clp/error_handling + src/clp/regex_utils + src/clp/string_utils +) +foreach(module ${CORE_UTILITY_MODULES}) + add_subdirectory(${module}) +endforeach()components/core/src/clp/error_handling/ErrorCode.hpp (1)
9-135: Consider adjusting the namespace usage for better extensibilityThe current design requires that specializations and customizations be placed within the
clp::error_handlingnamespace. This might be counter-intuitive for users working in other namespaces. Consider refactoring the code to allow users to define their specializations in their own namespaces, possibly leveraging argument-dependent lookup (ADL) or template techniques to reduce coupling.components/core/tests/test-error_handling.cpp (1)
83-87: Use explicit captures in lambda expressions for clarityIn the lambda expression within
std::any_of, consider capturing only the necessary variable explicitly. Since onlyconditionis used from the enclosing scope, you can modify the capture list to[&condition]. This improves readability and prevents unintended captures.Apply this diff to update the capture list:
- [&](auto failure_condition) -> bool { return condition == failure_condition; } + [&condition](auto failure_condition) -> bool { return condition == failure_condition; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/core/.clang-format(1 hunks)components/core/CMakeLists.txt(2 hunks)components/core/src/clp/error_handling/CMakeLists.txt(1 hunks)components/core/src/clp/error_handling/ErrorCode.hpp(1 hunks)components/core/tests/test-error_handling.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/core/.clang-format
🔇 Additional comments (4)
components/core/src/clp/error_handling/CMakeLists.txt (1)
5-6: LGTM! Well-structured library definition
The INTERFACE library setup with a namespaced alias follows CMake best practices for header-only libraries.
components/core/CMakeLists.txt (1)
464-464: LGTM! Test file properly integrated.
The test file for error handling is correctly added to the unit test suite and will be compiled with the appropriate dependencies.
components/core/src/clp/error_handling/ErrorCode.hpp (1)
143-147:
Incorrect specialization of std::is_error_code_enum
The macro CLP_ERROR_HANDLING_MARK_AS_ERROR_CODE_ENUM incorrectly specializes std::is_error_code_enum for clp::error_handling::ErrorCode<T>. According to standard practice, you should specialize std::is_error_code_enum for the enum type T directly. This enables implicit conversions from your enum to std::error_code.
Apply this diff to fix the specialization:
#define CLP_ERROR_HANDLING_MARK_AS_ERROR_CODE_ENUM(T) \
template <> \
- struct std::is_error_code_enum<clp::error_handling::ErrorCode<T>> : std::true_type { \
+ struct std::is_error_code_enum<T> : std::true_type { \
static_assert(std::is_enum_v<T>); \
};Likely invalid or redundant comment.
components/core/tests/test-error_handling.cpp (1)
94-140: Overall, the test cases are comprehensive and well-structured
The test suite thoroughly validates the functionality of the custom error codes and categories. It ensures correct behaviour when comparing error codes within the same category and across different categories. The use of Catch2 assertions effectively checks the expected outcomes.
| target_sources(error_handling INTERFACE ${ERROR_HANDLING_HEADER_LIST}) | ||
| target_include_directories(error_handling INTERFACE ../) | ||
| target_compile_features(error_handling INTERFACE cxx_std_20) |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Relative include directory ../ may cause issues
The current include path using ../ can lead to header inclusion problems in downstream projects.
🔗 Analysis chain
Verify include directory configuration
The include directory configuration using ../ might lead to header inclusion issues in downstream projects.
Also, consider the following improvements:
- Use
CMAKE_CURRENT_SOURCE_DIRfor more explicit include path - Add install configuration for the header files
-target_include_directories(error_handling INTERFACE ../)
+target_include_directories(error_handling INTERFACE
+ $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/..>
+ $<INSTALL_INTERFACE:include>
+)
+
+install(
+ FILES ${ERROR_HANDLING_HEADER_LIST}
+ DESTINATION include/clp/error_handling
+)
+
+install(
+ TARGETS error_handling
+ EXPORT error_handling
+)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any other CMake files that might be affected by the relative include path
# Look for other CMakeLists.txt files that include this directory
rg -l "add_subdirectory.*error_handling" --type cmake
# Check for any existing include directory configurations
rg "target_include_directories.*error_handling" --type cmake
Length of output: 260
| template <> | ||
| auto AlwaysSuccessErrorCategory::name() const noexcept -> char const* { | ||
| return cAlwaysSuccessErrorCategoryName.data(); | ||
| } | ||
|
|
||
| template <> | ||
| auto AlwaysSuccessErrorCategory::message(AlwaysSuccessErrorCodeEnum error_enum) const -> string { | ||
| switch (error_enum) { | ||
| case AlwaysSuccessErrorCodeEnum::Success: | ||
| return string{cSuccessErrorMsg}; | ||
| default: | ||
| return string{cUnrecognizedErrorCode}; | ||
| } | ||
| } | ||
|
|
||
| template <> | ||
| auto BinaryErrorCategory::name() const noexcept -> char const* { | ||
| return cBinaryTestErrorCategoryName.data(); | ||
| } | ||
|
|
||
| template <> | ||
| auto BinaryErrorCategory::message(BinaryErrorCodeEnum error_enum) const -> string { | ||
| switch (error_enum) { | ||
| case BinaryErrorCodeEnum::Success: | ||
| return string{cSuccessErrorMsg}; | ||
| case BinaryErrorCodeEnum::Failure: | ||
| return string{cFailureErrorMsg}; | ||
| default: | ||
| return string{cUnrecognizedErrorCode}; | ||
| } | ||
| } | ||
|
|
||
| template <> | ||
| auto BinaryErrorCategory::equivalent( | ||
| BinaryErrorCodeEnum error_enum, | ||
| std::error_condition const& condition | ||
| ) const noexcept -> bool { | ||
| switch (error_enum) { | ||
| case BinaryErrorCodeEnum::Failure: | ||
| return std::any_of( | ||
| cFailureConditions.cbegin(), | ||
| cFailureConditions.cend(), | ||
| [&](auto failure_condition) -> bool { return condition == failure_condition; } | ||
| ); | ||
| default: | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix namespaces of template specializations to comply with C++ standards
The explicit specializations of ErrorCategory methods (name(), message(), and equivalent()) must be defined in the same namespace as the primary template ErrorCategory, which is clp::error_handling. Defining these specializations outside their original namespace can lead to undefined behaviour and linkage issues. Please move these specializations into the clp::error_handling namespace to ensure standard compliance.
Apply this diff to move the specializations:
-template <>
-auto AlwaysSuccessErrorCategory::name() const noexcept -> char const* {
+template <>
+auto clp::error_handling::AlwaysSuccessErrorCategory::name() const noexcept -> char const* {
return cAlwaysSuccessErrorCategoryName.data();
}
-template <>
-auto AlwaysSuccessErrorCategory::message(AlwaysSuccessErrorCodeEnum error_enum) const -> string {
+template <>
+auto clp::error_handling::AlwaysSuccessErrorCategory::message(AlwaysSuccessErrorCodeEnum error_enum) const -> string {
// Implementation remains the same
}
-template <>
-auto BinaryErrorCategory::name() const noexcept -> char const* {
+template <>
+auto clp::error_handling::BinaryErrorCategory::name() const noexcept -> char const* {
return cBinaryTestErrorCategoryName.data();
}
-template <>
-auto BinaryErrorCategory::message(BinaryErrorCodeEnum error_enum) const -> string {
+template <>
+auto clp::error_handling::BinaryErrorCategory::message(BinaryErrorCodeEnum error_enum) const -> string {
// Implementation remains the same
}
-template <>
-auto BinaryErrorCategory::equivalent(
+template <>
+auto clp::error_handling::BinaryErrorCategory::equivalent(
BinaryErrorCodeEnum error_enum,
std::error_condition const& condition
) const noexcept -> bool {
// Implementation remains the same
}📝 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.
| template <> | |
| auto AlwaysSuccessErrorCategory::name() const noexcept -> char const* { | |
| return cAlwaysSuccessErrorCategoryName.data(); | |
| } | |
| template <> | |
| auto AlwaysSuccessErrorCategory::message(AlwaysSuccessErrorCodeEnum error_enum) const -> string { | |
| switch (error_enum) { | |
| case AlwaysSuccessErrorCodeEnum::Success: | |
| return string{cSuccessErrorMsg}; | |
| default: | |
| return string{cUnrecognizedErrorCode}; | |
| } | |
| } | |
| template <> | |
| auto BinaryErrorCategory::name() const noexcept -> char const* { | |
| return cBinaryTestErrorCategoryName.data(); | |
| } | |
| template <> | |
| auto BinaryErrorCategory::message(BinaryErrorCodeEnum error_enum) const -> string { | |
| switch (error_enum) { | |
| case BinaryErrorCodeEnum::Success: | |
| return string{cSuccessErrorMsg}; | |
| case BinaryErrorCodeEnum::Failure: | |
| return string{cFailureErrorMsg}; | |
| default: | |
| return string{cUnrecognizedErrorCode}; | |
| } | |
| } | |
| template <> | |
| auto BinaryErrorCategory::equivalent( | |
| BinaryErrorCodeEnum error_enum, | |
| std::error_condition const& condition | |
| ) const noexcept -> bool { | |
| switch (error_enum) { | |
| case BinaryErrorCodeEnum::Failure: | |
| return std::any_of( | |
| cFailureConditions.cbegin(), | |
| cFailureConditions.cend(), | |
| [&](auto failure_condition) -> bool { return condition == failure_condition; } | |
| ); | |
| default: | |
| return false; | |
| } | |
| } | |
| template <> | |
| auto clp::error_handling::AlwaysSuccessErrorCategory::name() const noexcept -> char const* { | |
| return cAlwaysSuccessErrorCategoryName.data(); | |
| } | |
| template <> | |
| auto clp::error_handling::AlwaysSuccessErrorCategory::message(AlwaysSuccessErrorCodeEnum error_enum) const -> string { | |
| switch (error_enum) { | |
| case AlwaysSuccessErrorCodeEnum::Success: | |
| return string{cSuccessErrorMsg}; | |
| default: | |
| return string{cUnrecognizedErrorCode}; | |
| } | |
| } | |
| template <> | |
| auto clp::error_handling::BinaryErrorCategory::name() const noexcept -> char const* { | |
| return cBinaryTestErrorCategoryName.data(); | |
| } | |
| template <> | |
| auto clp::error_handling::BinaryErrorCategory::message(BinaryErrorCodeEnum error_enum) const -> string { | |
| switch (error_enum) { | |
| case BinaryErrorCodeEnum::Success: | |
| return string{cSuccessErrorMsg}; | |
| case BinaryErrorCodeEnum::Failure: | |
| return string{cFailureErrorMsg}; | |
| default: | |
| return string{cUnrecognizedErrorCode}; | |
| } | |
| } | |
| template <> | |
| auto clp::error_handling::BinaryErrorCategory::equivalent( | |
| BinaryErrorCodeEnum error_enum, | |
| std::error_condition const& condition | |
| ) const noexcept -> bool { | |
| switch (error_enum) { | |
| case BinaryErrorCodeEnum::Failure: | |
| return std::any_of( | |
| cFailureConditions.cbegin(), | |
| cFailureConditions.cend(), | |
| [&](auto failure_condition) -> bool { return condition == failure_condition; } | |
| ); | |
| default: | |
| return false; | |
| } | |
| } |
There was a problem hiding this comment.
AlwaysSuccessErrorCode is an alias for ErrorCode<AlwaysSuccessErrorCodeEnum> (which is clp::error_handling::ErrorCode template), so auto AlwaysSuccessErrorCategory::name() const noexcept -> char const* should be equivalent to auto clp::error_handling::ErrorCode<AlwaysSuccessErrorCodeEnum>::name() const noexcept -> char const* unless I missed anything...
There was a problem hiding this comment.
@LinZhihao-723, thank you for the clarification. I understand your point now, and I'll keep that in mind for future reviews.
(^•ᴥ•^)
✏️ Learnings added
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/tests/test-error_handling.cpp:44-91
Timestamp: 2024-11-26T19:16:19.248Z
Learning: In `components/core/tests/test-error_handling.cpp` (C++), when specializing template methods of `ErrorCategory` via type aliases (e.g., `AlwaysSuccessErrorCategory`), it's acceptable to define these specializations in the current namespace, even though the primary template is in the `clp::error_handling` namespace, due to the use of type aliases and template instantiation.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Documentation: https://en.cppreference.com/w/cpp/language/template_specialization
namespace N
{
template<class T> // primary template
class X { /*...*/ };
template<> // specialization in same namespace
class X<int> { /*...*/ };
template<class T> // primary template
class Y { /*...*/ };
template<> // forward declare specialization for double
class Y<double>;
}
template<> // OK: specialization in same namespace
class N::Y<double> { /*...*/ };
There was a problem hiding this comment.
@LinZhihao-723, thank you for sharing the documentation link. This provides helpful clarification on template specializations. I'll make sure to consider this in future reviews.
(^・^)?
After reading the CMakes of gsl and msgpack, I feel like we're doing header-only cmake correctly in this PR for libraries, right? |
I haven't read their header library code yet, but I know they both have fundamental bugs in their CMake scripts, so I wouldn't trust them. |
LinZhihao-723
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
components/core/CMakeLists.txt (1)
322-322: Consider grouping error handling related filesFor better organization and maintainability, consider grouping the error handling header with its test file in a dedicated section of the source files list.
Apply this organization:
set(SOURCE_FILES_unitTest src/clp/Array.hpp + # Error handling components + src/clp/error_handling/ErrorCode.hpp + tests/test-error_handling.cpp src/clp/aws/AwsAuthenticationSigner.cpp # ... other files ... - src/clp/error_handling/ErrorCode.hpp # ... other files ... - tests/test-error_handling.cpp # ... remaining files ... )Also applies to: 505-505
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
components/core/CMakeLists.txt(2 hunks)components/core/tests/test-error_handling.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/core/tests/test-error_handling.cpp
🔇 Additional comments (1)
components/core/CMakeLists.txt (1)
322-322:
Add subdirectory for error handling components
The error handling system should follow the project's modular structure. Other utilities like regex_utils and string_utils have their own subdirectories with dedicated CMake configurations.
Add a new subdirectory configuration by creating src/clp/error_handling/CMakeLists.txt:
add_library(error_handling INTERFACE)
add_library(clp::error_handling ALIAS error_handling)
target_include_directories(error_handling
INTERFACE
${CMAKE_CURRENT_SOURCE_DIR}
)
target_compile_features(error_handling
INTERFACE
cxx_std_20
)Then, add the subdirectory to the main CMakeLists.txt:
add_subdirectory(src/clp/regex_utils)
add_subdirectory(src/clp/string_utils)
+add_subdirectory(src/clp/error_handling)⛔ Skipped due to learnings
Learnt from: LinZhihao-723
PR: y-scope/clp#486
File: components/core/src/clp/error_handling/ErrorCode.hpp:131-134
Timestamp: 2024-11-26T19:12:09.102Z
Learning: In the error handling system (`components/core/src/clp/error_handling/ErrorCode.hpp`), the `ErrorCode` class is intentionally designed to wrap the `ErrorCodeEnum`, not to use the enum directly. This wrapper provides a general interface for adding customized error enums.
|
Lgtm |
LinZhihao-723
changed the title
ErrorCode template to standardize conversion of user-defined error code enums to std::error_code.
…r-defined error code enums to `std::error_code`. (y-scope#486)
Description
Problem Statement
In the current CLP core, we use a generic enum defined in
clp/ErrorCode.hppas our error code machism. We've been seeing problems of this design from the following perspectives:ErrorCode_Unsupportedis used in both reader interfaces to indicate unsupported operations, and it is also used inWildcardTokento report an unsupported wildcard token. This error code itself doesn't provide the upper caller with the context of the error.ffi/ir_streamcomponent defines its own error enumsIRErrorCodeandIRProtocalErrorCode, instead of using the genericErrorCode_xxx. There is no systematic way to organize these different error code enums across the code base.std_resultfromoutcome, orstd::expectedin the future if we migrate to C++23.Solution
In modern C++, the standard does provide a systematic and type-safe way of managing error codes:
<system_error>. A detailed tutorial can be found in a cppcon talk here.The idea of this standard is simple. As module developers, you need to extend
std::error_codeto be compatible with a customizedstd::error_categoryand an enum of possible errors. Thestd::error_categorywill be used to identify the context of the errors with human-readable messages for each defined error. And for module users, they only need to compare the enum against thestd::error_codeinstance to inspect errors.Furthermore, the standard library also provide capabilities to define mappings from multiple error codes to one "error condition" using
std::error_condition, providing better management for sources of errors.By using this library, we should be able to solve all the problems mentioned in the previous section.
Implementation
This PR implements
ErrorCodeas a generic error code template, which integratesstd::error_codeand can be extended to customized error enums.To implement a new set of error codes, developers need to do the following steps:
MyErrorCodeEnumErrorCodeCategory<MyErrorCodeEnum>:name(): Defines the name of the error categorymessage(MyErrorCodeEnum error_enum): Provides a translation from the error enum to an error message stringstdnamespace, specialize the following template to be true:After these three steps,
ErrorCode<MyErrorCodeEnum>will be considered as a valid clp error code class. An instance of this class, initialized by an enum value fromMyErrorCodeEnum, will be considered as a domain-specific error. This error can be implicitly converted into astd::error_code, providing compatibilities withoutcome::std_resultand user-level error inspections.In addition to the template implementation, we also include a unit test to not only test the basic usage of the
ErrorCodetemplate, but also give an example of how to use the template to define a set of errors of a specific domain.Last but not least, the current implementation should be extendable to add error condition supports in the future.
Validation performed
std::error_codehandling introduced there.Summary by CodeRabbit
New Features
Bug Fixes