feat(error_handling): Migrate TraceableException from CLP with changes:
#43
Conversation
WalkthroughThis update introduces new components to the error handling library. A new exception class, TraceableException, is defined with constructors, methods for error and location reporting, and a macro to facilitate subclass creation. Additionally, a utility header provides an overload for streaming source location details. The build configuration is modified to publish these headers and include a new test source to validate the exception's functionality through a Worker class and an exception capture mechanism. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Catch2 Test
participant Worker
participant Exception as TraceableException
Test->>Worker: Call execute_with_success()
Worker-->>Test: Throw TraceableException (success)
Test->>Worker: Call execute_with_failure()
Worker-->>Test: Throw TraceableException (failure with details)
Test->>Worker: Invoke capture_exception(lambda)
Worker-->>Test: Return captured exception details
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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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
🧹 Nitpick comments (5)
src/ystdlib/error_handling/TraceableException.hpp (2)
33-70: Consider renaming the non-const variant ofwhat().
Having twowhat()methods with different return types may introduce confusion, even though they serve distinct purposes. A more descriptive name for the non-const variant can improve clarity.
74-82: Macro is helpful but could be replaced by a modern template-based approach.
While the macro expedites the creation of new exception types, leveraging an alias template or a derived class might enhance type safety and readability.src/ystdlib/error_handling/SourceLocation.hpp (1)
30-36: Clarify which file_name() method is being called.In
format_location,where.file_name()could refer to either the overridden method or the base class method depending on whether theYSTDLIB_CPP_PROJECT_SOURCE_PATH_SIZEmacro is defined. Consider explicitly calling the intended method to avoid confusion.static auto format_location(SourceLocation const& where) -> std::string { std::ostringstream oss; - oss << where.file_name() << "(" << where.line() << ":" << where.column() << "), function `" + oss << where.std::source_location::file_name() << "(" << where.line() << ":" << where.column() << "), function `" << where.function_name() << "`"; return oss.str(); }Or if you intend to use the potentially overridden method:
static auto format_location(SourceLocation const& where) -> std::string { std::ostringstream oss; + // Intentionally using the potentially overridden file_name() method oss << where.file_name() << "(" << where.line() << ":" << where.column() << "), function `" << where.function_name() << "`"; return oss.str(); }src/ystdlib/error_handling/test/test_TraceableException.cpp (2)
42-55: Forward declaration may be unnecessary.The forward declaration of
capture_exceptionat line 44 appears unnecessary since the full definition follows immediately in the same namespace. Consider removing it unless it serves a specific purpose.namespace { -template <ErrorCodeType E, typename Callable> -[[nodiscard]] auto capture_exception(Callable&& f) -> TraceableException<E>; - template <ErrorCodeType E, typename Callable> auto capture_exception(Callable&& f) -> TraceableException<E> { try { std::forward<Callable>(f)(); } catch (TraceableException<E>& e) { return e; } assert(false && "The function is expected to throw."); } } // namespace
57-67: Consider adding verification of error codes.While the test verifies file name, function name, and custom error message, it doesn't verify that the error codes are correctly preserved. Consider adding assertions to check that the error codes match the expected values.
TEST_CASE("test_traceable_exception", "[error_handling][TraceableException]") { auto const ex_success{capture_exception<BinaryErrorCode>(Worker::execute_with_success)}; REQUIRE((0 == std::strcmp(ex_success.where().file_name(), cCurrentFileName))); REQUIRE((0 == std::strcmp(ex_success.where().function_name(), cSuccessFuncName))); + REQUIRE(ex_success.error_code().value() == BinaryErrorCodeEnum::Success); auto const ex_failure{capture_exception<BinaryErrorCode>(Worker::execute_with_failure)}; REQUIRE((0 == std::strcmp(ex_failure.what(), cCustomFailureDescription))); REQUIRE((0 == std::strcmp(ex_failure.where().file_name(), cCurrentFileName))); REQUIRE((0 == std::strcmp(ex_failure.where().function_name(), cFailureFuncName))); + REQUIRE(ex_failure.error_code().value() == BinaryErrorCodeEnum::Failure); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CMakeLists.txt(1 hunks)src/ystdlib/error_handling/CMakeLists.txt(1 hunks)src/ystdlib/error_handling/SourceLocation.hpp(1 hunks)src/ystdlib/error_handling/TraceableException.hpp(1 hunks)src/ystdlib/error_handling/test/test_TraceableException.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/ystdlib/error_handling/SourceLocation.hppsrc/ystdlib/error_handling/TraceableException.hppsrc/ystdlib/error_handling/test/test_TraceableException.cpp
🧬 Code Definitions (2)
src/ystdlib/error_handling/TraceableException.hpp (2)
src/ystdlib/error_handling/SourceLocation.hpp (4)
where(31-36)where(31-31)nodiscard(18-21)nodiscard(24-24)src/ystdlib/error_handling/test/test_TraceableException.cpp (1)
YSTDLIB_ERROR_HANDLING_DEFINE_TRACEABLE_EXCEPTION(27-27)
src/ystdlib/error_handling/test/test_TraceableException.cpp (2)
src/ystdlib/error_handling/TraceableException.hpp (8)
TraceableException(37-44)TraceableException(37-40)TraceableException(46-53)TraceableException(46-50)nodiscard(56-56)nodiscard(59-59)nodiscard(61-61)nodiscard(63-63)src/ystdlib/error_handling/SourceLocation.hpp (2)
nodiscard(18-21)nodiscard(24-24)
🔇 Additional comments (12)
src/ystdlib/error_handling/CMakeLists.txt (2)
6-7: Public header additions look good.
These lines correctly add the newTraceableException.hppandSourceLocation.hppto the library's public headers.
11-11: Test source addition is appropriate.
Includingtest/test_TraceableException.cppensures coverage for newly introduced functionality.CMakeLists.txt (2)
45-49: Use ofstring(LENGTH ...)to capture source path size is correct.
This approach robustly obtains the project source directory length with trailing slash, preventing path-joining errors.
54-60: Conditional definition of the source path size macro is well-structured.
DefiningYSTDLIB_CPP_PROJECT_SOURCE_PATH_SIZEonly if present avoids unexpected build issues.src/ystdlib/error_handling/TraceableException.hpp (2)
1-15: File guards and includes look properly set up.
The header guard and essential includes are well placed, ensuring consistent compilation boundaries.
18-20: Concept definition is clear and robust.
Constraining template parameters tostd::error_codeor types convertible to it is concise and safe.src/ystdlib/error_handling/SourceLocation.hpp (2)
1-8: Header inclusions and guards look good.The file includes all necessary headers and properly defines include guards.
9-16: Good class inheritance implementation.The
SourceLocationclass properly inherits fromstd::source_locationand initializes the base class in the constructor. The member initialization usingformat_location(*this)is a clean approach to initialize the formatted string during construction.src/ystdlib/error_handling/test/test_TraceableException.cpp (4)
1-12: Includes and using declarations look good.All necessary headers are included and using declarations are properly applied.
13-22: Constants are well-defined and organized.The use of an anonymous namespace for test constants is a good practice. The constants for file names and function names are clearly defined.
24-39: Worker class implementation looks good.The
Workerclass properly demonstrates both exception throwing scenarios - with and without a custom error message.
60-66: Good use of coding guidelines.The assertions correctly follow the coding guideline of
false == <expression>rather than!<expression>by using0 == std::strcmp(...)instead of!std::strcmp(...).
…Update unit tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/ystdlib/error_handling/TraceableException.hpp (2)
26-65: Good implementation of the TraceableException template class.The exception class effectively captures error codes along with source location context, making it much easier to debug errors. The design with template parameter for ErrorType is flexible.
A few observations:
- The non-const
what()method allows modifying the exception message after construction, which could lead to unexpected behavior if used incorrectly.- The
what()const method returns a C-string pointer that could become invalid if the exception object is modified through the non-constwhat()method.Consider whether the non-const
what()method is necessary, or if it should be renamed to something likemodify_message()to make its purpose clearer.
71-81: Consider adding parentheses in the macro definitions.The current macro definitions could potentially cause issues with operator precedence. Consider adding parentheses around parameters in the macros.
For example:
- #define YSTDLIB_ERROR_HANDLING_DEFINE_TRACEABLE_EXCEPTION(T) \ + #define YSTDLIB_ERROR_HANDLING_DEFINE_TRACEABLE_EXCEPTION(T) \ class T : public ystdlib::error_handling::TraceableException<> { \ using ystdlib::error_handling::TraceableException<>::TraceableException; \ }This would help avoid potential issues if the macro is used with complex expressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/ystdlib/error_handling/CMakeLists.txt(1 hunks)src/ystdlib/error_handling/TraceableException.hpp(1 hunks)src/ystdlib/error_handling/test/test_TraceableException.cpp(1 hunks)src/ystdlib/error_handling/types.hpp(1 hunks)src/ystdlib/error_handling/utils.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ystdlib/error_handling/test/test_TraceableException.cpp
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/ystdlib/error_handling/utils.hppsrc/ystdlib/error_handling/types.hppsrc/ystdlib/error_handling/TraceableException.hpp
🧬 Code Definitions (1)
src/ystdlib/error_handling/TraceableException.hpp (1)
src/ystdlib/error_handling/test/test_TraceableException.cpp (1)
YSTDLIB_ERROR_HANDLING_DEFINE_TRACEABLE_EXCEPTION(28-28)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
🔇 Additional comments (5)
src/ystdlib/error_handling/types.hpp (1)
7-14: LGTM! Clean concept definition for error code types.The
ErrorCodeTypeconcept is well-defined, allowing bothstd::error_codeand convertible types. This will provide strong compile-time guarantees for template code that needs to work with error codes.src/ystdlib/error_handling/utils.hpp (1)
7-11: Good stream operator implementation for std::source_location.The operator provides a clear, formatted output of the source location components. This will be useful for debugging and error reporting.
src/ystdlib/error_handling/CMakeLists.txt (2)
6-8: LGTM! New headers properly added to PUBLIC_HEADERS.The TraceableException.hpp, types.hpp, and utils.hpp headers are correctly added to the library's public interface.
12-12: LGTM! Test file correctly added.The test_TraceableException.cpp file has been properly added to the TESTS_SOURCES section.
src/ystdlib/error_handling/TraceableException.hpp (1)
71-81: NOLINT issues in macros could be addressed.The macros are currently marked with NOLINT for specific issues. Consider addressing these issues in a future update.
Bill-hbrhbr
changed the title
TraceableException that leverages std::source_location.TraceableException that leverages std::source_location to propagate errors with contextual information.
Bill-hbrhbr
changed the title
TraceableException that leverages std::source_location to propagate errors with contextual information.TraceableException for propagating error codes with context across call layers.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/ystdlib/error_handling/TraceableException.hpp (3)
28-37: Consider clarifying the generated 'what' string.
While usingstd::ostringstreamto capture thestd::source_locationis convenient, it may not be obvious what the string contains (file, function, or both). For clarity, consider separating lines or labelling the data to make it more human-readable, especially if logs or client code rely on parsing these messages.
39-46: Confirm uniform handling of error messages.
When using the second constructor,m_whatis directly assigned the custom message. Consider ensuring consistency so that even custom messages may optionally include location details or other helpful context if desired.
69-73: Investigate removing the NOLINT directives.
The macro currently suppresses warnings related to macro usage. If possible, fix any underlying issues to remove the lint suppression. Otherwise, document the rationale for keeping these directives.src/ystdlib/error_handling/test/test_TraceableException.cpp (2)
26-37: Validate use of operation-specific exception.
ThrowingOperationFailedwithBinaryErrorCodeEnum::Successmight be counterintuitive if the name implies failure. Consider renaming or adding a success-specific exception if your domain logic demands clarity around success paths.
57-66: Consider verifyingerror_code().
The tests confirmwhat()and location data but do not verify thaterror_code().value()matchesBinaryErrorCodeEnum::SuccessorBinaryErrorCodeEnum::Failure. Adding checks would ensure the exception’s error code is properly set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/ystdlib/error_handling/CMakeLists.txt(1 hunks)src/ystdlib/error_handling/TraceableException.hpp(1 hunks)src/ystdlib/error_handling/test/test_TraceableException.cpp(1 hunks)src/ystdlib/error_handling/utils.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ystdlib/error_handling/utils.hpp
- src/ystdlib/error_handling/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
src/ystdlib/error_handling/TraceableException.hppsrc/ystdlib/error_handling/test/test_TraceableException.cpp
🧬 Code Definitions (2)
src/ystdlib/error_handling/TraceableException.hpp (1)
src/ystdlib/error_handling/test/test_TraceableException.cpp (1)
YSTDLIB_ERROR_HANDLING_DEFINE_TRACEABLE_EXCEPTION(26-26)
src/ystdlib/error_handling/test/test_TraceableException.cpp (1)
src/ystdlib/error_handling/TraceableException.hpp (8)
TraceableException(28-37)TraceableException(28-31)TraceableException(39-46)TraceableException(39-43)nodiscard(49-49)nodiscard(52-52)nodiscard(54-54)nodiscard(56-56)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint-check (ubuntu-24.04)
- GitHub Check: lint-check (macos-15)
| [[nodiscard]] auto what() const noexcept -> char const* override { return m_what.c_str(); } | ||
|
|
||
| // Methods | ||
| [[nodiscard]] auto error_code() const -> std::error_code { return m_error_code; } | ||
|
|
||
| [[nodiscard]] auto what() -> std::string& { return m_what; } | ||
|
|
||
| [[nodiscard]] auto where() const noexcept -> std::source_location const& { return m_where; } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid overloading 'what' with differing return types.
Having both what() const noexcept -> char const* (line 49) and what() -> std::string& (line 54) can cause confusion. Callers might expect the standard signature only. If read–write access to m_what is needed, consider offering a different named method.
| namespace { | ||
| template <typename Callable> | ||
| [[nodiscard]] auto capture_exception(Callable&& f) -> TraceableException; | ||
|
|
||
| template <typename Callable> | ||
| auto capture_exception(Callable&& f) -> TraceableException { | ||
| try { | ||
| std::forward<Callable>(f)(); | ||
| } catch (TraceableException& e) { | ||
| return e; | ||
| } | ||
| assert(false && "The function is expected to throw."); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety with template constraints.
The capture_exception template returns a TraceableException and expects that exact type to be thrown. If a different type of exception is actually thrown, the code falls through to assert(false). Consider constraining your template to accept callables that are guaranteed to throw TraceableException or handle unexpected exception types more gracefully.
davidlion
changed the title
TraceableException for propagating error codes with context across call layers.TraceableException from CLP with improvements:
davidlion
changed the title
TraceableException from CLP with improvements:TraceableException from CLP with changes:
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
feat(error_handling): Migrate TraceableException from CLP with changes:
- Replace
__FILENAME__and__LINE__macros withstd::source_location. - Add a new parameter
messageto allow users to specify custom error messages. - Add
YSTDLIB_ERROR_HANDLING_DEFINE_TRACEABLE_EXCEPTION(type_name)macro for simplified definitions. - Use absolute paths instead of relative paths for file locations.
Description
Migrate
TraceableExceptiontoystdlib-cppand makes the following changes:__FILENAME__and__LINE__macros withstd::source_location.throw OperationFailed(error_code);while still getting the file and line number at the call site due tostd::source_locationdefault function parameter substitution.std::source_locationalso provides the fully qualified call site function name, which helps us to eliminate duplicatestd::exception::what()overriding specifications that simply repeat the operation classes and/or their namespaces.messageto allow users to specify custom error messages.what()overrides allows users greater flexibility in defining custom error messages.YSTDLIB_ERROR_HANDLING_DEFINE_TRACEABLE_EXCEPTION(type_name)macro for simplified definitions.TraceableExceptionto a one-liner.OperationFailedwithYSTDLIB_ERROR_HANDLING_DEFINE_TRACEABLE_EXCEPTION(OperationFailed)ystdlib::error_handling::TraceableExceptionmay be used by multiple projects in various locations within the filesystem, it is not always possible to calculate the relative path of a source file.TraceableExceptionclass.Checklist
breaking change.
Validation performed
test_TraceableException.cpppasses.Summary by CodeRabbit
Summary by CodeRabbit
New Features
TraceableExceptionclass for enhanced error handling with detailed diagnostic context.Tests
TraceableExceptionclass, validating its functionality and context capture.