Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Add ErrorCode template to standardize conversion of user-defined error code enums to std::error_code. #486

Merged
merged 13 commits into from
Dec 2, 2024
4 changes: 2 additions & 2 deletions components/core/.clang-format
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ IncludeCategories:
# NOTE: A header is grouped by first matching regex
# Library headers. Update when adding new libraries.
# NOTE: clang-format retains leading white-space on a line in violation of the YAML spec.
- Regex: "<(absl|antlr4|archive|boost|bsoncxx|catch2|curl|date|fmt|json|log_surgeon|mariadb\
|mongocxx|msgpack|outcome|simdjson|spdlog|sqlite3|string_utils|yaml-cpp|zstd)"
- Regex: "<(absl|antlr4|archive|boost|bsoncxx|catch2|curl|date|error_handling|fmt|json\
|log_surgeon|mariadb|mongocxx|msgpack|outcome|simdjson|spdlog|sqlite3|string_utils|yaml-cpp|zstd)"
Priority: 3
# C system headers
- Regex: "^<.+\\.h>"
Expand Down
2 changes: 2 additions & 0 deletions components/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ set(sqlite_DYNAMIC_LIBS "dl;m;pthread")
include(cmake/Modules/FindLibraryDependencies.cmake)
FindDynamicLibraryDependencies(sqlite "${sqlite_DYNAMIC_LIBS}")

add_subdirectory(src/clp/error_handling)
add_subdirectory(src/clp/regex_utils)
add_subdirectory(src/clp/string_utils)

Expand Down Expand Up @@ -460,6 +461,7 @@ set(SOURCE_FILES_unitTest
tests/test-BufferedFileReader.cpp
tests/test-EncodedVariableInterpreter.cpp
tests/test-encoding_methods.cpp
tests/test-error_handling.cpp
tests/test-ffi_SchemaTree.cpp
tests/test-Grep.cpp
tests/test-ir_encoding_methods.cpp
Expand Down
9 changes: 9 additions & 0 deletions components/core/src/clp/error_handling/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
set(
ERROR_HANDLING_HEADER_LIST
"ErrorCode.hpp"
)
add_library(error_handling INTERFACE)
add_library(clp::error_handling ALIAS error_handling)
target_sources(error_handling INTERFACE ${ERROR_HANDLING_HEADER_LIST})
target_include_directories(error_handling INTERFACE ../)
target_compile_features(error_handling INTERFACE cxx_std_20)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. Use CMAKE_CURRENT_SOURCE_DIR for more explicit include path
  2. 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

137 changes: 137 additions & 0 deletions components/core/src/clp/error_handling/ErrorCode.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
#ifndef CLP_ERROR_HANDLING_ERRORCODE_HPP
#define CLP_ERROR_HANDLING_ERRORCODE_HPP

#include <concepts>
#include <string>
#include <system_error>
#include <type_traits>

namespace clp::error_handling {
/**
* Concept that defines a template parameter of an integer-based error code enumeration.
* @tparam Type
*/
template <typename Type>
concept ErrorCodeEnumType = std::is_enum_v<Type> && requires(Type type) {
{
static_cast<std::underlying_type_t<Type>>(type)
} -> std::convertible_to<int>;
};

/**
* Template that defines a `std::error_category` of the given set of error code enumeration.
* @tparam ErrorCodeEnum
*/
template <ErrorCodeEnumType ErrorCodeEnum>
class ErrorCategory : public std::error_category {
public:
// Methods implementing `std::error_category`
/**
* Gets the error category name.
* Note: A specialization must be explicitly implemented for each valid `ErrorCodeEnum`.
* @return The name of the error category.
*/
[[nodiscard]] auto name() const noexcept -> char const* override;
LinZhihao-723 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Gets the descriptive message associated with the given error.
* @param error_num
* @return The descriptive message for the error.
*/
[[nodiscard]] auto message(int error_num) const -> std::string override {
return message(static_cast<ErrorCodeEnum>(error_num));
}

/**
* @param error_num
* @param condition
* @return Whether the error condition of the given error matches the given condition.
*/
[[nodiscard]] auto equivalent(
int error_num,
std::error_condition const& condition
) const noexcept -> bool override {
return equivalent(static_cast<ErrorCodeEnum>(error_num), condition);
}

// Methods
/**
* Gets the descriptive message associated with the given error.
* Note: A specialization must be explicitly implemented for each valid `ErrorCodeEnum`.
* @param error_enum.
* @return The descriptive message for the error.
*/
[[nodiscard]] auto message(ErrorCodeEnum error_enum) const -> std::string;

/**
* Note: A specialization can be implemented to create error enum to error condition mappings.
* @param error_num
* @param condition
* @return Whether the error condition of the given error matches the given condition.
*/
[[nodiscard]] auto equivalent(
ErrorCodeEnum error_enum,
std::error_condition const& condition
) const noexcept -> bool;
};

/**
* Template class that defines an error code. An error code is represented by a error enum value and
* the associated error category. This template class is designed to be `std::error_code`
* compatible, meaning that every instance of this class can be used to construct a corresponded
* `std::error_code` instance, or compare with a `std::error_code` instance to inspect a specific
* error.
* @tparam ErrorCodeEnum
*/
template <ErrorCodeEnumType ErrorCodeEnum>
class ErrorCode {
public:
// Constructor
ErrorCode(ErrorCodeEnum error) : m_error{error} {}

/**
* @return The underlying error code enum.
*/
[[nodiscard]] auto get_error() const -> ErrorCodeEnum { return m_error; }

/**
* @return The error code as an error number.
*/
[[nodiscard]] auto get_error_num() const -> int { return static_cast<int>(m_error); }

/**
* @return The reference to the singleton of the corresponded error category.
*/
[[nodiscard]] constexpr static auto get_category() -> ErrorCategory<ErrorCodeEnum> const& {
return cCategory;
}

private:
static inline ErrorCategory<ErrorCodeEnum> const cCategory;

ErrorCodeEnum m_error;
};

/**
* @tparam ErrorCodeEnum
* @param error
* @return Constructed `std::error_code` from the given `ErrorCode` instance.
*/
template <typename ErrorCodeEnum>
[[nodiscard]] auto make_error_code(ErrorCode<ErrorCodeEnum> error) -> std::error_code;

template <ErrorCodeEnumType ErrorCodeEnum>
auto ErrorCategory<ErrorCodeEnum>::equivalent(
ErrorCodeEnum error_enum,
std::error_condition const& condition
) const noexcept -> bool {
return std::error_category::default_error_condition(static_cast<int>(error_enum)) == condition;
}

template <typename ErrorCodeEnum>
auto make_error_code(ErrorCode<ErrorCodeEnum> error) -> std::error_code {
return {error.get_error_num(), ErrorCode<ErrorCodeEnum>::get_category()};
}
LinZhihao-723 marked this conversation as resolved.
Show resolved Hide resolved
} // namespace clp::error_handling

#endif // CLP_ERROR_HANDLING_ERRORCODE_HPP
145 changes: 145 additions & 0 deletions components/core/tests/test-error_handling.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
#include <algorithm>
#include <array>
#include <cstdint>
#include <string>
#include <string_view>
#include <system_error>
#include <type_traits>

#include <Catch2/single_include/catch2/catch.hpp>
#include <error_handling/ErrorCode.hpp>

using clp::error_handling::ErrorCategory;
using clp::error_handling::ErrorCode;
using std::string;
using std::string_view;

namespace {
enum class AlwaysSuccessErrorCodeEnum : uint8_t {
Success = 0
};

enum class BinaryErrorCodeEnum : uint8_t {
Success = 0,
Failure
};

using AlwaysSuccessErrorCode = ErrorCode<AlwaysSuccessErrorCodeEnum>;
using AlwaysSuccessErrorCategory = ErrorCategory<AlwaysSuccessErrorCodeEnum>;
using BinaryErrorCode = ErrorCode<BinaryErrorCodeEnum>;
using BinaryErrorCategory = ErrorCategory<BinaryErrorCodeEnum>;

constexpr string_view cAlwaysSuccessErrorCategoryName{"Always Success Error Code"};
constexpr string_view cBinaryTestErrorCategoryName{"Binary Error Code"};
constexpr string_view cSuccessErrorMsg{"Success"};
constexpr string_view cFailureErrorMsg{"Failure"};
constexpr string_view cUnrecognizedErrorCode{"Unrecognized Error Code"};
constexpr std::array cFailureConditions{std::errc::not_connected, std::errc::timed_out};
constexpr std::array cNoneFailureConditions{std::errc::broken_pipe, std::errc::address_in_use};
LinZhihao-723 marked this conversation as resolved.
Show resolved Hide resolved
} // namespace

namespace std {
template <>
struct is_error_code_enum<BinaryErrorCode> : std::true_type {};

template <>
struct is_error_code_enum<AlwaysSuccessErrorCode> : std::true_type {};
} // namespace std

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;
}
}
Comment on lines +45 to +92
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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;
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> { /*...*/ };

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

(^・^)?


TEST_CASE("test_error_code_implementation", "[error_handling][ErrorCode]") {
// Test error codes within the same error category
BinaryErrorCode const success{BinaryErrorCodeEnum::Success};
std::error_code const success_error_code{success};
REQUIRE((success == success_error_code));
REQUIRE((cSuccessErrorMsg == success_error_code.message()));
REQUIRE((BinaryErrorCode::get_category() == success_error_code.category()));
REQUIRE((cBinaryTestErrorCategoryName == success_error_code.category().name()));

BinaryErrorCode const failure{BinaryErrorCodeEnum::Failure};
std::error_code const failure_error_code{failure};
REQUIRE((failure == failure_error_code));
REQUIRE((cFailureErrorMsg == failure_error_code.message()));
REQUIRE((BinaryErrorCode::get_category() == failure_error_code.category()));
REQUIRE((cBinaryTestErrorCategoryName == failure_error_code.category().name()));
std::for_each(
cFailureConditions.cbegin(),
cFailureConditions.cend(),
[&](auto failure_condition) { REQUIRE((failure_error_code == failure_condition)); }
);
std::for_each(
cNoneFailureConditions.cbegin(),
cNoneFailureConditions.cend(),
[&](auto none_failure_condition) {
REQUIRE((failure_error_code != none_failure_condition));
}
);

REQUIRE((success_error_code != failure_error_code));
REQUIRE((success_error_code.category() == failure_error_code.category()));

AlwaysSuccessErrorCode const always_success{AlwaysSuccessErrorCodeEnum::Success};
std::error_code const always_success_error_code{always_success};
REQUIRE((always_success_error_code == always_success));
REQUIRE((cSuccessErrorMsg == always_success_error_code.message()));
REQUIRE((AlwaysSuccessErrorCode::get_category() == always_success_error_code.category()));
REQUIRE((cAlwaysSuccessErrorCategoryName == always_success_error_code.category().name()));

// Compare error codes from different error category
// Error codes that have the same value or message won't be the same with each other if they are
// from different error categories.
REQUIRE((success_error_code.value() == always_success_error_code.value()));
REQUIRE((success_error_code.message() == always_success_error_code.message()));
REQUIRE((success_error_code.category() != always_success_error_code.category()));
REQUIRE((success_error_code != always_success_error_code));
REQUIRE((AlwaysSuccessErrorCode{AlwaysSuccessErrorCodeEnum::Success} != success_error_code));
REQUIRE((BinaryErrorCode{BinaryErrorCodeEnum::Success} != always_success_error_code));
}
Loading