Merge pull request #2146 from elBoberido/iox-1032-refactor-iox-require

elBoberido · web-flow · commit 9b5bcba428f4 · 2024-01-03T01:24:54.000+01:00
iox-#1032 Refactor 'IOX_REQUIRE'
diff --git a/doc/design/error_reporting.md b/doc/design/error_reporting.md
@@ -165,7 +165,7 @@ the case that it does not hold
 ```cpp
 int x;
 // ...
-IOX_REQUIRE(x>=0, Code::OutOfBounds);
+IOX_REQUIRE(x>=0, "required condition violation message");
 ```
 
 The condition is required to hold and this requirement is always checked.
diff --git a/iceoryx_hoofs/reporting/include/iox/assertions.hpp b/iceoryx_hoofs/reporting/include/iox/assertions.hpp
@@ -44,19 +44,29 @@
         iox::er::forwardPanic(CURRENT_SOURCE_LOCATION, message);                                                       \
     } while (false)
 
-/// @brief report fatal error if expr evaluates to false
-/// @note for conditions that may actually happen during correct use
-/// @param expr boolean expression that must hold
-/// @param error error object (or code)
-#define IOX_REQUIRE(expr, error) IOX_REPORT_FATAL_IF(!(expr), error)
-
 //************************************************************************************************
 //* For documentation of intent, defensive programming and debugging
 //*
 //* There are no error codes/errors required here on purpose, as it would make the use cumbersome.
 //* Instead a special internal error type is used.
 //************************************************************************************************
 
+/// @brief report fatal required condition violation if expr evaluates to false
+/// @note for conditions that may actually happen during correct use
+/// @param expr boolean expression that must hold
+/// @param message message to be forwarded in case of violation
+#define IOX_REQUIRE(expr, message)                                                                                     \
+    do                                                                                                                 \
+    {                                                                                                                  \
+        if (!(expr))                                                                                                   \
+        {                                                                                                              \
+            iox::er::forwardFatalError(iox::er::Violation::createRequiredConditionViolation(),                         \
+                                       iox::er::REQUIRED_CONDITION_VIOLATION,                                          \
+                                       CURRENT_SOURCE_LOCATION,                                                        \
+                                       message); /* @todo iox-#1032 add strigified 'expr' as '#expr' */                \
+        }                                                                                                              \
+    } while (false)
+
 /// @brief if enabled: report fatal precondition violation if expr evaluates to false
 /// @param expr boolean expression that must hold upon entry of the function it appears in
 /// @param message message to be forwarded in case of violation
diff --git a/iceoryx_hoofs/reporting/include/iox/error_reporting/custom/default/error_reporting_impl.hpp b/iceoryx_hoofs/reporting/include/iox/error_reporting/custom/default/error_reporting_impl.hpp
@@ -67,6 +67,8 @@ template <class Message>
     panic();
 }
 
+/// @todo iox-#1032 add a 'StringifiedCondition' template parameter
+
 // Report any error, general version.
 template <class Kind, class Error>
 inline void report(const SourceLocation& location, Kind, const Error& error)
@@ -103,46 +105,67 @@ inline void report(const SourceLocation& location, iox::er::FatalKind kind, cons
     h.onReportError(ErrorDescriptor(location, code, module));
 }
 
-template <class Error>
-inline void report(const SourceLocation& location, iox::er::PreconditionViolationKind kind, const Error& error)
+namespace detail
+{
+enum class NoMessage
+{
+};
+
+template <class Kind, class Error, class Message>
+inline void report(const SourceLocation& location, Kind kind, const Error& error, Message&& msg)
 {
     auto code = toCode(error);
     auto module = toModule(error);
-    IOX_ERROR_INTERNAL_LOG_FATAL(location, kind.name);
+    if constexpr (std::is_same<NoMessage, Message>::value)
+    {
+        IOX_ERROR_INTERNAL_LOG_FATAL(location, kind.name);
+    }
+    else
+    {
+        IOX_ERROR_INTERNAL_LOG_FATAL(location, kind.name << " " << std::forward<Message>(msg));
+    }
     auto& h = ErrorHandler::get();
     h.onReportViolation(ErrorDescriptor(location, code, module));
 }
+} // namespace detail
+
+template <class Error>
+inline void report(const SourceLocation& location, iox::er::RequiredConditionViolationKind kind, const Error& error)
+{
+    detail::report(location, kind, error, detail::NoMessage{});
+}
+
+template <class Error>
+inline void report(const SourceLocation& location, iox::er::PreconditionViolationKind kind, const Error& error)
+{
+    detail::report(location, kind, error, detail::NoMessage{});
+}
 
 template <class Error>
 inline void report(const SourceLocation& location, iox::er::AssumptionViolationKind kind, const Error& error)
 {
-    auto code = toCode(error);
-    auto module = toModule(error);
-    IOX_ERROR_INTERNAL_LOG_FATAL(location, kind.name);
-    auto& h = ErrorHandler::get();
-    h.onReportViolation(ErrorDescriptor(location, code, module));
+    detail::report(location, kind, error, detail::NoMessage{});
+}
+
+template <class Error, class Message>
+inline void
+report(const SourceLocation& location, iox::er::RequiredConditionViolationKind kind, const Error& error, Message&& msg)
+{
+    detail::report(location, kind, error, std::forward<Message>(msg));
 }
 
 template <class Error, class Message>
 inline void
 report(const SourceLocation& location, iox::er::PreconditionViolationKind kind, const Error& error, Message&& msg)
 {
-    auto code = toCode(error);
-    auto module = toModule(error);
-    IOX_ERROR_INTERNAL_LOG_FATAL(location, kind.name << " " << std::forward<Message>(msg));
-    auto& h = ErrorHandler::get();
-    h.onReportViolation(ErrorDescriptor(location, code, module));
+    detail::report(location, kind, error, std::forward<Message>(msg));
 }
 
 template <class Error, class Message>
 inline void
 report(const SourceLocation& location, iox::er::AssumptionViolationKind kind, const Error& error, Message&& msg)
 {
-    auto code = toCode(error);
-    auto module = toModule(error);
-    IOX_ERROR_INTERNAL_LOG_FATAL(location, kind.name << " " << std::forward<Message>(msg));
-    auto& h = ErrorHandler::get();
-    h.onReportViolation(ErrorDescriptor(location, code, module));
+    detail::report(location, kind, error, std::forward<Message>(msg));
 }
 
 } // namespace er
diff --git a/iceoryx_hoofs/reporting/include/iox/error_reporting/error_kind.hpp b/iceoryx_hoofs/reporting/include/iox/error_reporting/error_kind.hpp
@@ -37,6 +37,11 @@ struct FatalKind
     static constexpr char const* name = "Fatal Error";
 };
 
+struct RequiredConditionViolationKind
+{
+    static constexpr char const* name = "Required Condition Violation";
+};
+
 struct PreconditionViolationKind
 {
     static constexpr char const* name = "Precondition Violation";
@@ -62,6 +67,11 @@ struct IsFatal<FatalKind> : public std::true_type
 {
 };
 
+template <>
+struct IsFatal<RequiredConditionViolationKind> : public std::true_type
+{
+};
+
 template <>
 struct IsFatal<PreconditionViolationKind> : public std::true_type
 {
@@ -86,6 +96,12 @@ bool constexpr isFatal<FatalKind>(FatalKind)
     return IsFatal<FatalKind>::value;
 }
 
+template <>
+bool constexpr isFatal<RequiredConditionViolationKind>(RequiredConditionViolationKind)
+{
+    return IsFatal<RequiredConditionViolationKind>::value;
+}
+
 template <>
 bool constexpr isFatal<PreconditionViolationKind>(PreconditionViolationKind)
 {
@@ -101,6 +117,9 @@ bool constexpr isFatal<AssumptionViolationKind>(AssumptionViolationKind)
 // indicates serious condition, unable to continue
 constexpr FatalKind FATAL;
 
+// indicates a bug (contract breach by caller)
+constexpr RequiredConditionViolationKind REQUIRED_CONDITION_VIOLATION;
+
 // indicates a bug (contract breach by caller)
 constexpr PreconditionViolationKind PRECONDITION_VIOLATION;
 
diff --git a/iceoryx_hoofs/reporting/include/iox/error_reporting/errors.hpp b/iceoryx_hoofs/reporting/include/iox/error_reporting/errors.hpp
@@ -70,6 +70,11 @@ class Violation
         return !(*this == rhs);
     }
 
+    static Violation createRequiredConditionViolation()
+    {
+        return Violation(ErrorCode(ErrorCode::REQUIRED_CONDITION_VIOLATION));
+    }
+
     static Violation createPreconditionViolation()
     {
         return Violation(ErrorCode(ErrorCode::PRECONDITION_VIOLATION));
diff --git a/iceoryx_hoofs/reporting/include/iox/error_reporting/macros.hpp b/iceoryx_hoofs/reporting/include/iox/error_reporting/macros.hpp
@@ -61,7 +61,10 @@
     {                                                                                                                  \
         if (expr)                                                                                                      \
         {                                                                                                              \
-            iox::er::forwardNonFatalError(iox::er::toError(error), kind, CURRENT_SOURCE_LOCATION);                     \
+            iox::er::forwardNonFatalError(                                                                             \
+                iox::er::toError(error),                                                                               \
+                kind,                                                                                                  \
+                CURRENT_SOURCE_LOCATION); /* @todo iox-#1032 add strigified 'expr' as '#expr' */                       \
         }                                                                                                              \
     } while (false)
 
diff --git a/iceoryx_hoofs/reporting/include/iox/error_reporting/types.hpp b/iceoryx_hoofs/reporting/include/iox/error_reporting/types.hpp
@@ -33,8 +33,9 @@ struct ErrorCode
 
     type value;
 
-    static constexpr type ASSUMPTION_VIOLATION{0};
-    static constexpr type PRECONDITION_VIOLATION{1};
+    static constexpr type REQUIRED_CONDITION_VIOLATION{0};
+    static constexpr type ASSUMPTION_VIOLATION{1};
+    static constexpr type PRECONDITION_VIOLATION{2};
 
     constexpr explicit ErrorCode(uint32_t value)
         : value(value)
diff --git a/iceoryx_hoofs/test/moduletests/error_reporting/test_error_reporting_macros.cpp b/iceoryx_hoofs/test/moduletests/error_reporting/test_error_reporting_macros.cpp
@@ -128,25 +128,25 @@ TEST_F(ErrorReportingMacroApi_test, reportConditionalNoError)
     IOX_TESTING_EXPECT_OK();
 }
 
-TEST_F(ErrorReportingMacroApi_test, requireConditionSatisfied)
+TEST_F(ErrorReportingMacroApi_test, checkRequireConditionSatisfied)
 {
     ::testing::Test::RecordProperty("TEST_ID", "3c684878-20f8-426f-bb8b-7576b567d04f");
-    auto f = []() { IOX_REQUIRE(true, MyCodeA::OutOfBounds); };
+    auto f = []() { IOX_REQUIRE(true, ""); };
 
     runInTestThread(f);
 
     IOX_TESTING_EXPECT_OK();
 }
 
-TEST_F(ErrorReportingMacroApi_test, requireConditionNotSatisfied)
+TEST_F(ErrorReportingMacroApi_test, checkRequireConditionViolate)
 {
     ::testing::Test::RecordProperty("TEST_ID", "fb62d315-8854-401b-82af-6161ae45a34e");
-    auto f = []() { IOX_REQUIRE(false, MyCodeA::OutOfBounds); };
+    auto f = []() { IOX_REQUIRE(false, ""); };
 
     runInTestThread(f);
 
     IOX_TESTING_EXPECT_PANIC();
-    IOX_TESTING_EXPECT_ERROR(MyCodeA::OutOfBounds);
+    IOX_TESTING_EXPECT_REQUIRED_CONDITION_VIOLATION();
 }
 
 TEST_F(ErrorReportingMacroApi_test, checkPreconditionSatisfied)
diff --git a/iceoryx_hoofs/testing/error_reporting/testing_support.cpp b/iceoryx_hoofs/testing/error_reporting/testing_support.cpp
@@ -31,6 +31,12 @@ bool hasError()
     return ErrorHandler::instance().hasError();
 }
 
+bool hasRequiredConditionViolation()
+{
+    auto code = iox::er::ErrorCode{iox::er::ErrorCode::REQUIRED_CONDITION_VIOLATION};
+    return ErrorHandler::instance().hasViolation(code);
+}
+
 bool hasPreconditionViolation()
 {
     auto code = iox::er::ErrorCode{iox::er::ErrorCode::PRECONDITION_VIOLATION};
@@ -45,7 +51,7 @@ bool hasAssumptionViolation()
 
 bool hasViolation()
 {
-    return hasPreconditionViolation() || hasAssumptionViolation();
+    return hasRequiredConditionViolation() || hasPreconditionViolation() || hasAssumptionViolation();
 }
 
 bool isInNormalState()
diff --git a/iceoryx_hoofs/testing/include/iceoryx_hoofs/testing/error_reporting/testing_support.hpp b/iceoryx_hoofs/testing/include/iceoryx_hoofs/testing/error_reporting/testing_support.hpp
@@ -50,6 +50,9 @@ bool hasPanicked();
 /// @brief indicates whether the test error handler registered any error
 bool hasError();
 
+/// @brief indicates whether the test error handler registered a required condition violation
+bool hasRequiredConditionViolation();
+
 /// @brief indicates whether the test error handler registered a precondition violation
 bool hasPreconditionViolation();
 
@@ -120,11 +123,11 @@ inline void runInTestThread(Function&& testFunction, Args&&... args)
 
 #define IOX_TESTING_ASSERT_NO_ERROR() ASSERT_FALSE(iox::testing::hasError())
 
-#define IOX_TESTING_ASSERT_VIOLATION()                                                                                 \
-    ASSERT_TRUE(iox::testing::hasPreconditionViolation() || iox::testing::hasAssumptionViolation())
+#define IOX_TESTING_ASSERT_VIOLATION() ASSERT_TRUE(iox::testing::hasViolation())
+
+#define IOX_TESTING_ASSERT_NO_VIOLATION() ASSERT_FALSE(iox::testing::hasViolation())
 
-#define IOX_TESTING_ASSERT_NO_VIOLATION()                                                                              \
-    ASSERT_FALSE(iox::testing::hasPreconditionViolation() || iox::testing::hasAssumptionViolation())
+#define IOX_TESTING_ASSERT_REQUIRED_CONDITION_VIOLATION() ASSERT_TRUE(iox::testing::hasRequiredConditionViolation())
 
 #define IOX_TESTING_ASSERT_PRECONDITION_VIOLATION() ASSERT_TRUE(iox::testing::hasPreconditionViolation())
 
@@ -142,11 +145,11 @@ inline void runInTestThread(Function&& testFunction, Args&&... args)
 
 #define IOX_TESTING_EXPECT_NO_ERROR() EXPECT_FALSE(iox::testing::hasError())
 
-#define IOX_TESTING_EXPECT_VIOLATION()                                                                                 \
-    EXPECT_TRUE(iox::testing::hasPreconditionViolation() || iox::testing::hasAssumptionViolation())
+#define IOX_TESTING_EXPECT_VIOLATION() EXPECT_TRUE(iox::testing::hasViolation())
+
+#define IOX_TESTING_EXPECT_NO_VIOLATION() EXPECT_FALSE(iox::testing::hasViolation())
 
-#define IOX_TESTING_EXPECT_NO_VIOLATION()                                                                              \
-    EXPECT_FALSE(iox::testing::hasPreconditionViolation() || iox::testing::hasAssumptionViolation())
+#define IOX_TESTING_EXPECT_REQUIRED_CONDITION_VIOLATION() EXPECT_TRUE(iox::testing::hasRequiredConditionViolation())
 
 #define IOX_TESTING_EXPECT_PRECONDITION_VIOLATION() EXPECT_TRUE(iox::testing::hasPreconditionViolation())
 

Original file line number	Diff line number	Diff line change
`@@ -70,6 +70,11 @@ class Violation`
`70`	`70`	`return !(*this == rhs);`
`71`	`71`	`}`
`72`	`72`
	`73`	`+ static Violation createRequiredConditionViolation()`
	`74`	`+ {`
	`75`	`+ return Violation(ErrorCode(ErrorCode::REQUIRED_CONDITION_VIOLATION));`
	`76`	`+ }`
	`77`	`+`
`73`	`78`	`static Violation createPreconditionViolation()`
`74`	`79`	`{`
`75`	`80`	`return Violation(ErrorCode(ErrorCode::PRECONDITION_VIOLATION));`
Original file line number	Diff line number	Diff line change
`@@ -128,25 +128,25 @@ TEST_F(ErrorReportingMacroApi_test, reportConditionalNoError)`
`128`	`128`	`IOX_TESTING_EXPECT_OK();`
`129`	`129`	`}`
`130`	`130`
`131`		`-TEST_F(ErrorReportingMacroApi_test, requireConditionSatisfied)`
	`131`	`+TEST_F(ErrorReportingMacroApi_test, checkRequireConditionSatisfied)`
`132`	`132`	`{`
`133`	`133`	`::testing::Test::RecordProperty("TEST_ID", "3c684878-20f8-426f-bb8b-7576b567d04f");`
`134`		`- auto f = []() { IOX_REQUIRE(true, MyCodeA::OutOfBounds); };`
	`134`	`+ auto f = []() { IOX_REQUIRE(true, ""); };`
`135`	`135`
`136`	`136`	`runInTestThread(f);`
`137`	`137`
`138`	`138`	`IOX_TESTING_EXPECT_OK();`
`139`	`139`	`}`
`140`	`140`
`141`		`-TEST_F(ErrorReportingMacroApi_test, requireConditionNotSatisfied)`
	`141`	`+TEST_F(ErrorReportingMacroApi_test, checkRequireConditionViolate)`
`142`	`142`	`{`
`143`	`143`	`::testing::Test::RecordProperty("TEST_ID", "fb62d315-8854-401b-82af-6161ae45a34e");`
`144`		`- auto f = []() { IOX_REQUIRE(false, MyCodeA::OutOfBounds); };`
	`144`	`+ auto f = []() { IOX_REQUIRE(false, ""); };`
`145`	`145`
`146`	`146`	`runInTestThread(f);`
`147`	`147`
`148`	`148`	`IOX_TESTING_EXPECT_PANIC();`
`149`		`- IOX_TESTING_EXPECT_ERROR(MyCodeA::OutOfBounds);`
	`149`	`+ IOX_TESTING_EXPECT_REQUIRED_CONDITION_VIOLATION();`
`150`	`150`	`}`
`151`	`151`
`152`	`152`	`TEST_F(ErrorReportingMacroApi_test, checkPreconditionSatisfied)`
Original file line number	Diff line number	Diff line change
`@@ -31,6 +31,12 @@ bool hasError()`
`31`	`31`	`return ErrorHandler::instance().hasError();`
`32`	`32`	`}`
`33`	`33`
	`34`	`+bool hasRequiredConditionViolation()`
	`35`	`+{`
	`36`	`+ auto code = iox::er::ErrorCode{iox::er::ErrorCode::REQUIRED_CONDITION_VIOLATION};`
	`37`	`+ return ErrorHandler::instance().hasViolation(code);`
	`38`	`+}`
	`39`	`+`
`34`	`40`	`bool hasPreconditionViolation()`
`35`	`41`	`{`
`36`	`42`	`auto code = iox::er::ErrorCode{iox::er::ErrorCode::PRECONDITION_VIOLATION};`
`@@ -45,7 +51,7 @@ bool hasAssumptionViolation()`
`45`	`51`
`46`	`52`	`bool hasViolation()`
`47`	`53`	`{`
`48`		`- return hasPreconditionViolation() \|\| hasAssumptionViolation();`
	`54`	`+ return hasRequiredConditionViolation() \|\| hasPreconditionViolation() \|\| hasAssumptionViolation();`
`49`	`55`	`}`
`50`	`56`
`51`	`57`	`bool isInNormalState()`