From 3dd2e841c34bfe3d966ae6606f9b69d75f1a3442 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Mon, 4 Mar 2019 11:10:22 -0500 Subject: Googletest export Fix emission of -Wzero-as-null-pointer-constant when comparing integers. The following code fails to compile: #pragma clang diagnostic error "-Wzero-as-null-pointer-constant" void foo() { EXPECT_EQ(0, 0); } This happens because gtest checks the first argument to EXPECT_EQ and ASSERT_EQ is a null pointer constant. The magic it does to do this causes the warning to be emitted. This patch removes that check. It replaces the explicit check with a Compare overload that can only be selected when 0 or nullptr is passed on the LHS with a pointer on the right. This patch does not suppress -Wzero-as-null-pointer-constant when users are actually using it as NULL. PiperOrigin-RevId: 236654634 --- googletest/include/gtest/gtest.h | 63 +++++-------------- googletest/include/gtest/internal/gtest-internal.h | 31 ---------- googletest/test/gtest_unittest.cc | 70 ++++++++++++++++------ 3 files changed, 66 insertions(+), 98 deletions(-) diff --git a/googletest/include/gtest/gtest.h b/googletest/include/gtest/gtest.h index 5046f7dd..5211a20b 100644 --- a/googletest/include/gtest/gtest.h +++ b/googletest/include/gtest/gtest.h @@ -52,9 +52,11 @@ #ifndef GTEST_INCLUDE_GTEST_GTEST_H_ #define GTEST_INCLUDE_GTEST_GTEST_H_ +#include #include #include #include +#include #include #include "gtest/internal/gtest-internal.h" @@ -1532,18 +1534,17 @@ GTEST_API_ AssertionResult CmpHelperEQ(const char* lhs_expression, BiggestInt lhs, BiggestInt rhs); -// The helper class for {ASSERT|EXPECT}_EQ. The template argument -// lhs_is_null_literal is true iff the first argument to ASSERT_EQ() -// is a null pointer literal. The following default implementation is -// for lhs_is_null_literal being false. -template class EqHelper { public: // This templatized version is for the general case. - template + template < + typename T1, typename T2, + // Disable this overload for cases where one argument is a pointer + // and the other is the null pointer constant. + typename std::enable_if::value || + !std::is_pointer::value>::type* = nullptr> static AssertionResult Compare(const char* lhs_expression, - const char* rhs_expression, - const T1& lhs, + const char* rhs_expression, const T1& lhs, const T2& rhs) { return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs); } @@ -1560,44 +1561,12 @@ class EqHelper { BiggestInt rhs) { return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs); } -}; -// This specialization is used when the first argument to ASSERT_EQ() -// is a null pointer literal, like NULL, false, or 0. -template <> -class EqHelper { - public: - // We define two overloaded versions of Compare(). The first - // version will be picked when the second argument to ASSERT_EQ() is - // NOT a pointer, e.g. ASSERT_EQ(0, AnIntFunction()) or - // EXPECT_EQ(false, a_bool). - template - static AssertionResult Compare( - const char* lhs_expression, const char* rhs_expression, const T1& lhs, - const T2& rhs, - // The following line prevents this overload from being considered if T2 - // is not a pointer type. We need this because ASSERT_EQ(NULL, my_ptr) - // expands to Compare("", "", NULL, my_ptr), which requires a conversion - // to match the Secret* in the other overload, which would otherwise make - // this template match better. - typename EnableIf::value>::type* = nullptr) { - return CmpHelperEQ(lhs_expression, rhs_expression, lhs, rhs); - } - - // This version will be picked when the second argument to ASSERT_EQ() is a - // pointer, e.g. ASSERT_EQ(NULL, a_pointer). template static AssertionResult Compare( - const char* lhs_expression, - const char* rhs_expression, - // We used to have a second template parameter instead of Secret*. That - // template parameter would deduce to 'long', making this a better match - // than the first overload even without the first overload's EnableIf. - // Unfortunately, gcc with -Wconversion-null warns when "passing NULL to - // non-pointer argument" (even a deduced integral argument), so the old - // implementation caused warnings in user code. - Secret* /* lhs (NULL) */, - T* rhs) { + const char* lhs_expression, const char* rhs_expression, + // Handle cases where '0' is used as a null pointer literal. + std::nullptr_t /* lhs */, T* rhs) { // We already know that 'lhs' is a null pointer. return CmpHelperEQ(lhs_expression, rhs_expression, static_cast(nullptr), rhs); @@ -2046,9 +2015,7 @@ class TestWithParam : public Test, public WithParamInterface { // ASSERT_GT(records.size(), 0) << "There is no record left."; #define EXPECT_EQ(val1, val2) \ - EXPECT_PRED_FORMAT2(::testing::internal:: \ - EqHelper::Compare, \ - val1, val2) + EXPECT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) #define EXPECT_NE(val1, val2) \ EXPECT_PRED_FORMAT2(::testing::internal::CmpHelperNE, val1, val2) #define EXPECT_LE(val1, val2) \ @@ -2061,9 +2028,7 @@ class TestWithParam : public Test, public WithParamInterface { EXPECT_PRED_FORMAT2(::testing::internal::CmpHelperGT, val1, val2) #define GTEST_ASSERT_EQ(val1, val2) \ - ASSERT_PRED_FORMAT2(::testing::internal:: \ - EqHelper::Compare, \ - val1, val2) + ASSERT_PRED_FORMAT2(::testing::internal::EqHelper::Compare, val1, val2) #define GTEST_ASSERT_NE(val1, val2) \ ASSERT_PRED_FORMAT2(::testing::internal::CmpHelperNE, val1, val2) #define GTEST_ASSERT_LE(val1, val2) \ diff --git a/googletest/include/gtest/internal/gtest-internal.h b/googletest/include/gtest/internal/gtest-internal.h index 82d39da6..949d1ebe 100644 --- a/googletest/include/gtest/internal/gtest-internal.h +++ b/googletest/include/gtest/internal/gtest-internal.h @@ -124,37 +124,6 @@ class IgnoredValue { IgnoredValue(const T& /* ignored */) {} // NOLINT(runtime/explicit) }; -// The only type that should be convertible to Secret* is nullptr. -// The other null pointer constants are not of a type that is convertible to -// Secret*. Only the literal with the right value is. -template -using TypeIsValidNullptrConstant = std::integral_constant< - bool, std::is_same::type, std::nullptr_t>::value || - !std::is_convertible::value>; - -// Two overloaded helpers for checking at compile time whether an -// expression is a null pointer literal (i.e. NULL or any 0-valued -// compile-time integral constant). These helpers have no -// implementations, as we only need their signatures. -// -// Given IsNullLiteralHelper(x), the compiler will pick the first -// version if x can be implicitly converted to Secret*, and pick the -// second version otherwise. Since Secret is a secret and incomplete -// type, the only expression a user can write that has type Secret* is -// a null pointer literal. Therefore, we know that x is a null -// pointer literal if and only if the first version is picked by the -// compiler. -std::true_type IsNullLiteralHelper(Secret*, std::true_type); -std::false_type IsNullLiteralHelper(IgnoredValue, std::false_type); -std::false_type IsNullLiteralHelper(IgnoredValue, std::true_type); - -// A compile-time bool constant that is true if and only if x is a null pointer -// literal (i.e. nullptr, NULL or any 0-valued compile-time integral constant). -#define GTEST_IS_NULL_LITERAL_(x) \ - decltype(::testing::internal::IsNullLiteralHelper( \ - x, \ - ::testing::internal::TypeIsValidNullptrConstant()))::value - // Appends the user-supplied message to the Google-Test-generated message. GTEST_API_ std::string AppendUserMessage( const std::string& gtest_msg, const Message& user_msg); diff --git a/googletest/test/gtest_unittest.cc b/googletest/test/gtest_unittest.cc index 4ab298c4..69d35230 100644 --- a/googletest/test/gtest_unittest.cc +++ b/googletest/test/gtest_unittest.cc @@ -515,22 +515,23 @@ TEST_F(FormatEpochTimeInMillisAsIso8601Test, PrintsEpochStart) { # pragma option push -w-ccc -w-rch # endif -// Tests that GTEST_IS_NULL_LITERAL_(x) is true when x is a null -// pointer literal. -TEST(NullLiteralTest, IsTrueForNullLiterals) { - EXPECT_TRUE(GTEST_IS_NULL_LITERAL_(NULL)); // NOLINT - EXPECT_TRUE(GTEST_IS_NULL_LITERAL_(0)); // NOLINT - EXPECT_TRUE(GTEST_IS_NULL_LITERAL_(0u)); // NOLINT - EXPECT_TRUE(GTEST_IS_NULL_LITERAL_(nullptr)); -} - -// Tests that GTEST_IS_NULL_LITERAL_(x) is false when x is not a null -// pointer literal. -TEST(NullLiteralTest, IsFalseForNonNullLiterals) { - EXPECT_FALSE(GTEST_IS_NULL_LITERAL_(1)); - EXPECT_FALSE(GTEST_IS_NULL_LITERAL_(0.0)); - EXPECT_FALSE(GTEST_IS_NULL_LITERAL_('a')); - EXPECT_FALSE(GTEST_IS_NULL_LITERAL_(static_cast(nullptr))); +// Tests that the LHS of EXPECT_EQ or ASSERT_EQ can be used as a null literal +// when the RHS is a pointer type. +TEST(NullLiteralTest, LHSAllowsNullLiterals) { + EXPECT_EQ(0, static_cast(nullptr)); // NOLINT + ASSERT_EQ(0, static_cast(nullptr)); // NOLINT + EXPECT_EQ(NULL, static_cast(nullptr)); // NOLINT + ASSERT_EQ(NULL, static_cast(nullptr)); // NOLINT + EXPECT_EQ(nullptr, static_cast(nullptr)); + ASSERT_EQ(nullptr, static_cast(nullptr)); + + const int* const p = nullptr; + EXPECT_EQ(0, p); // NOLINT + ASSERT_EQ(0, p); // NOLINT + EXPECT_EQ(NULL, p); // NOLINT + ASSERT_EQ(NULL, p); // NOLINT + EXPECT_EQ(nullptr, p); + ASSERT_EQ(nullptr, p); } struct ConvertToAll { @@ -540,6 +541,13 @@ struct ConvertToAll { } }; +struct ConvertToPointer { + template + operator T*() const { // NOLINT + return nullptr; + } +}; + struct ConvertToAllButNoPointers { template ::value, int>::type = 0> @@ -548,11 +556,37 @@ struct ConvertToAllButNoPointers { } }; +struct MyType {}; +inline bool operator==(MyType const&, MyType const&) { return true; } + TEST(NullLiteralTest, ImplicitConversion) { - EXPECT_FALSE(GTEST_IS_NULL_LITERAL_(ConvertToAll{})); - EXPECT_FALSE(GTEST_IS_NULL_LITERAL_(ConvertToAllButNoPointers{})); + EXPECT_EQ(ConvertToPointer{}, static_cast(nullptr)); +#if !defined(__GNUC__) || defined(__clang__) + // Disabled due to GCC bug gcc.gnu.org/PR89580 + EXPECT_EQ(ConvertToAll{}, static_cast(nullptr)); +#endif + EXPECT_EQ(ConvertToAll{}, MyType{}); + EXPECT_EQ(ConvertToAllButNoPointers{}, MyType{}); +} + +#ifdef __clang__ +#pragma clang diagnostic push +#if __has_warning("-Wzero-as-null-pointer-constant") +#pragma clang diagnostic error "-Wzero-as-null-pointer-constant" +#endif +#endif + +TEST(NullLiteralTest, NoConversionNoWarning) { + // Test that gtests detection and handling of null pointer constants + // doesn't trigger a warning when '0' isn't actually used as null. + EXPECT_EQ(0, 0); + ASSERT_EQ(0, 0); } +#ifdef __clang__ +#pragma clang diagnostic pop +#endif + # ifdef __BORLANDC__ // Restores warnings after previous "#pragma option push" suppressed them. # pragma option pop -- cgit v1.2.3