From 389b1173f68e98338467d88c1e6ec1a112de4471 Mon Sep 17 00:00:00 2001 From: Wyatt Hepler Date: Fri, 30 Jul 2021 15:45:51 -0700 Subject: pw_assert: Use static_cast in PW_CHECK comparisons - Previously, the comparison forms of PW_CHECK used a C-style cast. This meant you could accidentally compare an argument to the PW_CHECK message on 32-bit platforms. For example: PW_CHECK_INT_EQ(123, "This was cast to int and compared to 123!"); Now, the macros use static_cast in C++, which causes compiler errors for incompatible types. - Support checking function pointers with PW_CHECK_NOTNULL. - Update tests that used PW_CHECK_PTR_EQ with integers. Change-Id: Iecbb67fb38c90d2511f540f8c92787f4050081b0 Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/55346 Commit-Queue: Wyatt Hepler Reviewed-by: Ewout van Bekkum --- pw_assert/assert_facade_test.cc | 93 +++++++++++++++++++----- pw_assert/public/pw_assert/internal/check_impl.h | 37 ++++++++-- 2 files changed, 104 insertions(+), 26 deletions(-) (limited to 'pw_assert') diff --git a/pw_assert/assert_facade_test.cc b/pw_assert/assert_facade_test.cc index 9606c7bd5..a2a6fdd50 100644 --- a/pw_assert/assert_facade_test.cc +++ b/pw_assert/assert_facade_test.cc @@ -194,38 +194,91 @@ TEST_F(AssertPass, UintGe3) { PW_CHECK_UINT_GE(2, 1); } // Test comparison boundaries. // PTR < -TEST_F(AssertPass, PtrLt1) { PW_CHECK_PTR_LT(0xa, 0xb); } -TEST_F(AssertFail, PtrLt2) { PW_CHECK_PTR_LT(0xb, 0xb); } -TEST_F(AssertFail, PtrLt3) { PW_CHECK_PTR_LT(0xb, 0xa); } +TEST_F(AssertPass, PtrLt1) { + PW_CHECK_PTR_LT(reinterpret_cast(0xa), reinterpret_cast(0xb)); +} +TEST_F(AssertFail, PtrLt2) { + PW_CHECK_PTR_LT(reinterpret_cast(0xb), reinterpret_cast(0xb)); +} +TEST_F(AssertFail, PtrLt3) { + PW_CHECK_PTR_LT(reinterpret_cast(0xb), reinterpret_cast(0xa)); +} // PTR <= -TEST_F(AssertPass, PtrLe1) { PW_CHECK_PTR_LE(0xa, 0xb); } -TEST_F(AssertPass, PtrLe2) { PW_CHECK_PTR_LE(0xb, 0xb); } -TEST_F(AssertFail, PtrLe3) { PW_CHECK_PTR_LE(0xb, 0xa); } +TEST_F(AssertPass, PtrLe1) { + PW_CHECK_PTR_LE(reinterpret_cast(0xa), reinterpret_cast(0xb)); +} +TEST_F(AssertPass, PtrLe2) { + PW_CHECK_PTR_LE(reinterpret_cast(0xb), reinterpret_cast(0xb)); +} +TEST_F(AssertFail, PtrLe3) { + PW_CHECK_PTR_LE(reinterpret_cast(0xb), reinterpret_cast(0xa)); +} // PTR == -TEST_F(AssertFail, PtrEq1) { PW_CHECK_PTR_EQ(0xa, 0xb); } -TEST_F(AssertPass, PtrEq2) { PW_CHECK_PTR_EQ(0xb, 0xb); } -TEST_F(AssertFail, PtrEq3) { PW_CHECK_PTR_EQ(0xb, 0xa); } +TEST_F(AssertFail, PtrEq1) { + PW_CHECK_PTR_EQ(reinterpret_cast(0xa), reinterpret_cast(0xb)); +} +TEST_F(AssertPass, PtrEq2) { + PW_CHECK_PTR_EQ(reinterpret_cast(0xb), reinterpret_cast(0xb)); +} +TEST_F(AssertFail, PtrEq3) { + PW_CHECK_PTR_EQ(reinterpret_cast(0xb), reinterpret_cast(0xa)); +} // PTR != -TEST_F(AssertPass, PtrNe1) { PW_CHECK_PTR_NE(0xa, 0xb); } -TEST_F(AssertFail, PtrNe2) { PW_CHECK_PTR_NE(0xb, 0xb); } -TEST_F(AssertPass, PtrNe3) { PW_CHECK_PTR_NE(0xb, 0xa); } +TEST_F(AssertPass, PtrNe1) { + PW_CHECK_PTR_NE(reinterpret_cast(0xa), reinterpret_cast(0xb)); +} +TEST_F(AssertFail, PtrNe2) { + PW_CHECK_PTR_NE(reinterpret_cast(0xb), reinterpret_cast(0xb)); +} +TEST_F(AssertPass, PtrNe3) { + PW_CHECK_PTR_NE(reinterpret_cast(0xb), reinterpret_cast(0xa)); +} // PTR > -TEST_F(AssertFail, PtrGt1) { PW_CHECK_PTR_GT(0xa, 0xb); } -TEST_F(AssertFail, PtrGt2) { PW_CHECK_PTR_GT(0xb, 0xb); } -TEST_F(AssertPass, PtrGt3) { PW_CHECK_PTR_GT(0xb, 0xa); } +TEST_F(AssertFail, PtrGt1) { + PW_CHECK_PTR_GT(reinterpret_cast(0xa), reinterpret_cast(0xb)); +} +TEST_F(AssertFail, PtrGt2) { + PW_CHECK_PTR_GT(reinterpret_cast(0xb), reinterpret_cast(0xb)); +} +TEST_F(AssertPass, PtrGt3) { + PW_CHECK_PTR_GT(reinterpret_cast(0xb), reinterpret_cast(0xa)); +} // PTR >= -TEST_F(AssertFail, PtrGe1) { PW_CHECK_PTR_GE(0xa, 0xb); } -TEST_F(AssertPass, PtrGe2) { PW_CHECK_PTR_GE(0xb, 0xb); } -TEST_F(AssertPass, PtrGe3) { PW_CHECK_PTR_GE(0xb, 0xa); } +TEST_F(AssertFail, PtrGe1) { + PW_CHECK_PTR_GE(reinterpret_cast(0xa), reinterpret_cast(0xb)); +} +TEST_F(AssertPass, PtrGe2) { + PW_CHECK_PTR_GE(reinterpret_cast(0xb), reinterpret_cast(0xb)); +} +TEST_F(AssertPass, PtrGe3) { + PW_CHECK_PTR_GE(reinterpret_cast(0xb), reinterpret_cast(0xa)); +} // NOTNULL -TEST_F(AssertPass, PtrNotNull) { PW_CHECK_NOTNULL(0xa); } -TEST_F(AssertFail, PtrNotNull) { PW_CHECK_NOTNULL(0x0); } +TEST_F(AssertPass, PtrNotNull) { + PW_CHECK_NOTNULL(reinterpret_cast(0xa)); +} +TEST_F(AssertFail, PtrNotNull) { + PW_CHECK_NOTNULL(reinterpret_cast(0x0)); +} + +[[maybe_unused]] void Function1() {} +[[maybe_unused]] bool Function2(int) { return false; } + +// NOTNULL for function poionters +TEST_F(AssertPass, FunctionPtrNotNull) { + PW_CHECK_NOTNULL(&Function1); + PW_CHECK_NOTNULL(&Function2); +} +TEST_F(AssertFail, FunctionPtrNotNull) { + void (*const function)() = nullptr; + PW_CHECK_NOTNULL(function); +} // Note: Due to platform inconsistencies, the below test for the NOTNULL // message doesn't work. Some platforms print NULL formatted as %p as "(nil)", diff --git a/pw_assert/public/pw_assert/internal/check_impl.h b/pw_assert/public/pw_assert/internal/check_impl.h index 5dba29209..4b8de6926 100644 --- a/pw_assert/public/pw_assert/internal/check_impl.h +++ b/pw_assert/public/pw_assert/internal/check_impl.h @@ -13,7 +13,9 @@ // the License. #pragma once -#ifndef __cplusplus +#ifdef __cplusplus +#include +#else #include #endif // __cplusplus @@ -248,6 +250,29 @@ type_fmt, \ __VA_ARGS__) +// Use a static_cast in C++ to avoid accidental comparisons between e.g. an +// integer and the CHECK message const char*. +#if defined(__cplusplus) && __cplusplus >= 201703L + +template +constexpr const void* ConvertToType(U* value) { + if constexpr (std::is_function()) { + return reinterpret_cast(value); + } else { + return static_cast(value); + } +} + +template +constexpr T ConvertToType(const U& value) { + return static_cast(value); +} + +#define _PW_CHECK_CONVERT(type, name, arg) type name = ConvertToType(arg) +#else +#define _PW_CHECK_CONVERT(type, name, arg) type name = (type)(arg) +#endif // __cplusplus + // For the binary assertions, this private macro is re-used for almost all of // the variants. Due to limitations of C formatting, it is necessary to have // separate macros for the types. @@ -255,15 +280,15 @@ // The macro avoids evaluating the arguments multiple times at the cost of some // macro complexity. #define _PW_CHECK_BINARY_CMP_IMPL( \ - argument_a, comparison_op, argument_b, type_decl, type_fmt, ...) \ + arg_a, comparison_op, arg_b, type_decl, type_fmt, ...) \ do { \ - type_decl evaluated_argument_a = (type_decl)(argument_a); \ - type_decl evaluated_argument_b = (type_decl)(argument_b); \ + _PW_CHECK_CONVERT(type_decl, evaluated_argument_a, arg_a); \ + _PW_CHECK_CONVERT(type_decl, evaluated_argument_b, arg_b); \ if (!(evaluated_argument_a comparison_op evaluated_argument_b)) { \ - _PW_CHECK_BINARY_COMPARISON_SELECT_MACRO(#argument_a, \ + _PW_CHECK_BINARY_COMPARISON_SELECT_MACRO(#arg_a, \ evaluated_argument_a, \ #comparison_op, \ - #argument_b, \ + #arg_b, \ evaluated_argument_b, \ type_fmt, \ PW_HAS_ARGS(__VA_ARGS__), \ -- cgit v1.2.3