pw_assert: PW_CHECK_OK() macro for Status This adds PW_CHECK_OK() for both C and C++ to assert that a Status object or enum is OK. Change-Id: I85e963bffc39510769feeb93e0b331f044557e7e
diff --git a/pw_assert/BUILD.gn b/pw_assert/BUILD.gn index 0fac58d..63b61b6 100644 --- a/pw_assert/BUILD.gn +++ b/pw_assert/BUILD.gn
@@ -48,6 +48,7 @@ "public/pw_assert/internal/assert_impl.h", "pw_assert_test/fake_backend.h", ] + deps = [ dir_pw_status ] } pw_test("assert_backend_compile_test") { @@ -55,6 +56,7 @@ deps = [ ":pw_assert", dir_pw_assert_backend, + dir_pw_status, ] sources = [ "assert_backend_compile_test.c",
diff --git a/pw_assert/assert_backend_compile_test.c b/pw_assert/assert_backend_compile_test.c index 599fa93..fb81a94 100644 --- a/pw_assert/assert_backend_compile_test.c +++ b/pw_assert/assert_backend_compile_test.c
@@ -24,6 +24,7 @@ #define PW_ASSERT_USE_SHORT_NAMES 1 #include "pw_assert/assert.h" +#include "pw_status/status.h" #ifdef __cplusplus #error "This file must be compiled as plain C to verify C compilation works." @@ -182,4 +183,13 @@ CHECK_INT_LE(Add3(1, 2, 3), Add3(1, 2, 3), "INT: " FAIL_IF_DISPLAYED); CHECK_INT_LE(x_int, y_int, "INT: " FAIL_IF_DISPLAYED_ARGS, z); } + + { // Compile tests for PW_ASSERT_OK(). + PW_CHECK_OK(PW_STATUS_OK); + PW_CHECK_OK(PW_STATUS_OK, "msg"); + PW_CHECK_OK(PW_STATUS_OK, "msg: %d", 5); + PW_DCHECK_OK(PW_STATUS_OK); + PW_DCHECK_OK(PW_STATUS_OK, "msg"); + PW_DCHECK_OK(PW_STATUS_OK, "msg: %d", 5); + } }
diff --git a/pw_assert/assert_backend_compile_test.cc b/pw_assert/assert_backend_compile_test.cc index 47df056..372810b 100644 --- a/pw_assert/assert_backend_compile_test.cc +++ b/pw_assert/assert_backend_compile_test.cc
@@ -40,6 +40,7 @@ #include "pw_assert/assert.h" // clang-format on +#include "pw_status/status.h" #include "gtest/gtest.h" // This is a global constant to feed into the formatter for tests. @@ -197,6 +198,32 @@ CHECK_INT_LE(x_int, y_int, "INT: " FAIL_IF_DISPLAYED_ARGS, z); } +pw::Status MakeStatus(pw::Status status) { return status; } + +TEST(Check, CheckOkMacrosCompile) { + MAYBE_SKIP_TEST; + pw::Status status = pw::Status::UNKNOWN; + + // Typical case with long names. + PW_CHECK_OK(status); + PW_CHECK_OK(status, "msg"); + PW_CHECK_OK(status, "msg: %d", 5); + + // Short names. + CHECK_OK(status); + CHECK_OK(status, "msg"); + CHECK_OK(status, "msg: %d", 5); + + // Status from a literal. + PW_CHECK_OK(pw::Status::OK); + + // Status from a function. + PW_CHECK_OK(MakeStatus(pw::Status::OK)); + + // Status from C enums. + PW_CHECK_OK(PW_STATUS_OK); +} + // These are defined in assert_test.c, to test C compatibility. extern "C" void AssertBackendCompileTestsInC();
diff --git a/pw_assert/assert_facade_test.cc b/pw_assert/assert_facade_test.cc index c4b0890..a234562 100644 --- a/pw_assert/assert_facade_test.cc +++ b/pw_assert/assert_facade_test.cc
@@ -420,6 +420,65 @@ CHECK_INT_LE(1, 2, "msg: %d", 5); } +// Verify PW_CHECK_OK, including message handling. +TEST_F(AssertFail, StatusNotOK) { + pw::Status status = pw::Status::UNKNOWN; + PW_CHECK_OK(status); + EXPECT_MESSAGE("Check failed: status (=UNKNOWN) == Status::OK (=OK). "); +} + +TEST_F(AssertFail, StatusNotOKMessageNoArguments) { + pw::Status status = pw::Status::UNKNOWN; + PW_CHECK_OK(status, "msg"); + EXPECT_MESSAGE("Check failed: status (=UNKNOWN) == Status::OK (=OK). msg"); +} + +TEST_F(AssertFail, StatusNotOKMessageArguments) { + pw::Status status = pw::Status::UNKNOWN; + PW_CHECK_OK(status, "msg: %d", 5); + EXPECT_MESSAGE("Check failed: status (=UNKNOWN) == Status::OK (=OK). msg: 5"); +} + +// Example expression for the test below. +pw::Status DoTheThing() { return pw::Status::RESOURCE_EXHAUSTED; } + +TEST_F(AssertFail, NonTrivialExpression) { + PW_CHECK_OK(DoTheThing()); + EXPECT_MESSAGE( + "Check failed: DoTheThing() (=RESOURCE_EXHAUSTED) == Status::OK (=OK). "); +} + +// Note: This function seems pointless but it is not, since pw::Status::FOO +// constants are not actually status objects, but code objects. This way we can +// ensure the macros work with both real status objects and literals. +pw::Status MakeStatus(pw::Status status) { return status; } +TEST_F(AssertPass, Constant) { PW_CHECK_OK(pw::Status::OK); } +TEST_F(AssertPass, Dynamic) { PW_CHECK_OK(MakeStatus(pw::Status::OK)); } +TEST_F(AssertPass, Enum) { PW_CHECK_OK(PW_STATUS_OK); } +TEST_F(AssertFail, Constant) { PW_CHECK_OK(pw::Status::UNKNOWN); } +TEST_F(AssertFail, Dynamic) { PW_CHECK_OK(MakeStatus(pw::Status::UNKNOWN)); } +TEST_F(AssertFail, Enum) { PW_CHECK_OK(PW_STATUS_UNKNOWN); } + +#if PW_ASSERT_ENABLE_DCHECK + +// In debug mode, the asserts should check their arguments. +TEST_F(AssertPass, DCheckConstant) { PW_DCHECK_OK(pw::Status::OK); } +TEST_F(AssertPass, DCheckDynamic) { PW_DCHECK_OK(MakeStatus(pw::Status::OK)); } +TEST_F(AssertFail, DCheckConstant) { PW_DCHECK_OK(pw::Status::UNKNOWN); } +TEST_F(AssertFail, DCheckDynamic) { + PW_DCHECK_OK(MakeStatus(pw::Status::UNKNOWN)); +} +#else // PW_ASSERT_ENABLE_DCHECK + +// In release mode, all the asserts should pass. +TEST_F(AssertPass, DCheckConstant) { PW_DCHECK_OK(pw::Status::OK); } +TEST_F(AssertPass, DCheckDynamic) { PW_DCHECK_OK(MakeStatus(pw::Status::OK)); } +TEST_F(AssertPass, DCheckConstant) { PW_DCHECK_OK(pw::Status::UNKNOWN); } +TEST_F(AssertPass, DCheckDynamic) { + PW_DCHECK_OK(MakeStatus(pw::Status::UNKNOWN)); +} +#endif // PW_ASSERT_ENABLE_DCHECK + // TODO: Figure out how to run some of these tests is C. } // namespace
diff --git a/pw_assert/docs.rst b/pw_assert/docs.rst index 5608043..b3c6452 100644 --- a/pw_assert/docs.rst +++ b/pw_assert/docs.rst
@@ -174,22 +174,29 @@ .. attention:: - Don't use use ``PW_CHECK`` for binary comparisons! + Don't use use ``PW_CHECK`` for binary comparisons or status checks! Instead, use the ``PW_CHECK_<TYPE>_<OP>`` macros. These macros enable capturing the value of the operands, and also tokenizing them if using a tokenizing assert backend. For example, if ``x`` and ``b`` are integers, use instead ``PW_CHECK_INT_LT(x, b)``. - +---------------------------------+-------------------------------------+ - | **Do NOT do this** | **Do this instead** | - +---------------------------------+-------------------------------------+ - | ``PW_CHECK(a_int < b_int)`` | ``PW_CHECK_INT_LT(a_int, b_int)`` | - +---------------------------------+-------------------------------------+ - | ``PW_CHECK(a_ptr <= b_ptr)`` | ``PW_CHECK_PTR_LE(a_ptr, b_ptr)`` | - +---------------------------------+-------------------------------------+ - | ``PW_CHECK(Temp() <= 10.0)`` | ``PW_CHECK_FLOAT_LE(Temp(), 10.0)`` | - +---------------------------------+-------------------------------------+ + Additionally, use ``PW_CHECK_OK(status)`` when checking for a + ``Status::OK``, since it enables showing a human-readable status string + rather than an integer (e.g. ``status == RESOURCE_EXHAUSTED`` instead of + ``status == 5``. + + +------------------------------------+-------------------------------------+ + | **Do NOT do this** | **Do this instead** | + +------------------------------------+-------------------------------------+ + | ``PW_CHECK(a_int < b_int)`` | ``PW_CHECK_INT_LT(a_int, b_int)`` | + +------------------------------------+-------------------------------------+ + | ``PW_CHECK(a_ptr <= b_ptr)`` | ``PW_CHECK_PTR_LE(a_ptr, b_ptr)`` | + +------------------------------------+-------------------------------------+ + | ``PW_CHECK(Temp() <= 10.0)`` | ``PW_CHECK_FLOAT_LE(Temp(), 10.0)`` | + +------------------------------------+-------------------------------------+ + | ``PW_CHECK(Foo() == Status::OK)`` | ``PW_CHECK_OK(Foo())`` | + +------------------------------------+-------------------------------------+ .. cpp:function:: PW_CHECK_NOTNULL(ptr) .. cpp:function:: PW_CHECK_NOTNULL(ptr, format, ...) @@ -207,7 +214,7 @@ Foo* foo = GetTheFoo() PW_CHECK_NOTNULL(foo); - Bar* bar = GetSomeBar() + Bar* bar = GetSomeBar(); PW_CHECK_NOTNULL(bar, "Weirdly got NULL bar; state: %d", MyState()); .. cpp:function:: PW_CHECK_TYPE_OP(a, b) @@ -348,6 +355,37 @@ | PW_DCHECK_FLOAT_NE | float | a != b | %f | +--------------------+--------------+-----------+-----------------------+ +.. cpp:function:: PW_CHECK_OK(status) +.. cpp:function:: PW_CHECK_OK(status, format, ...) +.. cpp:function:: PW_DCHECK_OK(status) +.. cpp:function:: PW_DCHECK_OK(status, format, ...) + + Assert that ``status`` evaluates to ``pw::Status::OK`` (in C++) or + ``PW_STATUS_OK`` (in C). Optionally include a message with arguments to + report. + + The ``DCHECK`` variants only run if ``NDEBUG`` is defined; otherwise, the + entire statement is removed (and the expression not evaluated). + + .. code-block:: cpp + + pw::Status operation_status = DoSomeOperation(); + PW_CHECK_OK(operation_status); + + // Any expression that evaluates to a pw::Status or pw_Status works. + PW_CHECK_OK(DoTheThing(), "System state: %s", SystemState()); + + // C works too. + pw_Status c_status = DoMoreThings(); + PW_CHECK_OK(c_status, "System state: %s", SystemState()); + + .. note:: + + Using ``PW_CHECK_OK(status)`` instead of ``PW_CHECK(status == Status::OK)`` + enables displaying an error message with a string version of the error + code; for example ``status == RESOURCE_EXHAUSTED`` instead of ``status == + 5``. + ---------------------------- Assert backend API reference ----------------------------
diff --git a/pw_assert/public/pw_assert/internal/assert_impl.h b/pw_assert/public/pw_assert/internal/assert_impl.h index a03c1d1..7814340 100644 --- a/pw_assert/public/pw_assert/internal/assert_impl.h +++ b/pw_assert/public/pw_assert/internal/assert_impl.h
@@ -128,6 +128,21 @@ // clang-format on +// PW_CHECK - If condition evaluates to false, crash. Message optional. +#define PW_CHECK_OK(status, ...) \ + do { \ + if (status != PW_STATUS_OK) { \ + _PW_CHECK_OK_SELECT_MACRO(#status, \ + pw_StatusString(status), \ + PW_HAS_ARGS(__VA_ARGS__), \ + __VA_ARGS__); \ + } \ + } while (0) + +#define PW_DCHECK_OK(...) \ + if (PW_ASSERT_ENABLE_DCHECK) \ + PW_CHECK_OK(__VA_ARGS__) + // ========================================================================= // Implementation for PW_CHECK @@ -223,8 +238,6 @@ // // The macro avoids evaluating the arguments multiple times at the cost of some // macro complexity. -// -// TODO: Concat names with __LINE__; requires an extra layer of macros. #define _PW_CHECK_BINARY_CMP_IMPL( \ argument_a, comparison_op, argument_b, type_decl, type_fmt, ...) \ do { \ @@ -242,14 +255,45 @@ } \ } while (0) +// ========================================================================= +// Implementation for PW_CHECK_OK + +// TODO: Explain why we must expand another time. +#define _PW_CHECK_OK_SELECT_MACRO( \ + status_expr_str, status_value_str, has_args, ...) \ + _PW_CHECK_OK_SELECT_MACRO_EXPANDED( \ + status_expr_str, status_value_str, has_args, __VA_ARGS__) + +// Delegate to the macro +#define _PW_CHECK_OK_SELECT_MACRO_EXPANDED( \ + status_expr_str, status_value_str, has_args, ...) \ + _PW_CHECK_OK_HAS_MSG_##has_args( \ + status_expr_str, status_value_str, __VA_ARGS__) + +// PW_CHECK_OK version 1: No message or args +#define _PW_CHECK_OK_HAS_MSG_0(status_expr_str, status_value_str, ignored_arg) \ + PW_HANDLE_ASSERT_BINARY_COMPARE_FAILURE( \ + status_expr_str, status_value_str, "==", "Status::OK", "OK", "%s", "") + +// PW_CHECK_OK version 2: With message (and maybe args) +#define _PW_CHECK_OK_HAS_MSG_1(status_expr_str, status_value_str, ...) \ + PW_HANDLE_ASSERT_BINARY_COMPARE_FAILURE(status_expr_str, \ + status_value_str, \ + "==", \ + "Status::OK", \ + "OK", \ + "%s", \ + __VA_ARGS__) + // Define short, usable names if requested. Note that the CHECK() macro will // conflict with Google Log, which expects stream style logs. -// -// TODO(pwbug/17): Convert this to the config system when available. #ifndef PW_ASSERT_USE_SHORT_NAMES #define PW_ASSERT_USE_SHORT_NAMES 0 #endif +// ========================================================================= +// Short name definitions (optional) + // clang-format off #if PW_ASSERT_USE_SHORT_NAMES @@ -281,6 +325,7 @@ #define CHECK_FLOAT_GT PW_CHECK_FLOAT_GT #define CHECK_FLOAT_EQ PW_CHECK_FLOAT_EQ #define CHECK_FLOAT_NE PW_CHECK_FLOAT_NE +#define CHECK_OK PW_CHECK_OK // Checks that are disabled if NDEBUG is not defined. #define DCHECK PW_DCHECK @@ -309,6 +354,7 @@ #define DCHECK_FLOAT_GT PW_DCHECK_FLOAT_GT #define DCHECK_FLOAT_EQ PW_DCHECK_FLOAT_EQ #define DCHECK_FLOAT_NE PW_DCHECK_FLOAT_NE +#define DCHECK_OK PW_DCHECK_OK #endif // PW_ASSERT_SHORT_NAMES // clang-format on
diff --git a/pw_assert/pw_assert_test/fake_backend.h b/pw_assert/pw_assert_test/fake_backend.h index c059056..ee7f3be 100644 --- a/pw_assert/pw_assert_test/fake_backend.h +++ b/pw_assert/pw_assert_test/fake_backend.h
@@ -67,9 +67,13 @@ // clang-format off // This is too hairy for clang format to handle and retain readability. -#define PW_HANDLE_ASSERT_BINARY_COMPARE_FAILURE( \ - arg_a_str, arg_a_val, comparison_op_str, arg_b_str, arg_b_val, \ - type_fmt, message, ...) \ +#define PW_HANDLE_ASSERT_BINARY_COMPARE_FAILURE(arg_a_str, \ + arg_a_val, \ + comparison_op_str, \ + arg_b_str, \ + arg_b_val, \ + type_fmt, \ + message, ...) \ pw_CaptureAssert(__FILE__, \ __LINE__, \ __func__, \