From dc51386b9fd61f99ea9c660d01867e6ad489b403 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Thu, 8 Feb 2024 22:21:18 +0100 Subject: [PATCH] Support literal-zero detectors using consteval int constructors This was originally motivated by `REQUIRE((a <=> b) == 0)` no longer compiling using MSVC. After some investigation, I found that they changed their implementation of the zero literal detector from the previous pointer-constructor with deleted other constructors, into one that uses `consteval` constructor from int. This breaks the previous detection logic, because now `is_foo_comparable` is true, but actually trying to compare them is a compile-time error... The solution was to make the decomposition `constexpr` and rely on a late C++20 DR that makes it so that `consteval` propagates up through the callstack of `constexpr` functions, until it either runs out of `constexpr` functions, or succeeds. However, the default handling of types in decomposition is to take a reference to them. This reference never becomes dangling, but because the constexpr evaluation engine cannot prove this, decomposition paths taking references to objects cannot be actually evaluated at compilation time. Thankfully we already did have a value-oriented decomposition path for arithmetic types (as these are common linkage-less types), so we could just explicitly spell out the `std::foo_ordering` types as also being supposed to be decomposed by-value. Two more fun facts about these changes 1) The original motivation of the MSVC change was to avoid trigering a `Wzero-as-null-pointer-constant` warning. I still do not believe this was a good decision. 2) Current latest version of MSVC does not actually implement the aforementioned C++20 DR, so even with this commit, MSVC cannot compile `REQUIRE((a <=> b) == 0)`. --- .../internal/catch_compiler_capabilities.hpp | 4 + src/catch2/internal/catch_decomposer.cpp | 7 +- src/catch2/internal/catch_decomposer.hpp | 90 +++++++++++++------ .../SelfTest/UsageTests/Compilation.tests.cpp | 71 ++++++++++++++- .../helpers/type_with_lit_0_comparisons.hpp | 23 +++-- 5 files changed, 154 insertions(+), 41 deletions(-) diff --git a/src/catch2/internal/catch_compiler_capabilities.hpp b/src/catch2/internal/catch_compiler_capabilities.hpp index dacae01b70..fc76937ce0 100644 --- a/src/catch2/internal/catch_compiler_capabilities.hpp +++ b/src/catch2/internal/catch_compiler_capabilities.hpp @@ -37,6 +37,10 @@ # define CATCH_CPP17_OR_GREATER # endif +# if (__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >= 202002L) +# define CATCH_CPP20_OR_GREATER +# endif + #endif // Only GCC compiler should be used in this block, so other compilers trying to diff --git a/src/catch2/internal/catch_decomposer.cpp b/src/catch2/internal/catch_decomposer.cpp index 3f398fcc2b..17a7bc9551 100644 --- a/src/catch2/internal/catch_decomposer.cpp +++ b/src/catch2/internal/catch_decomposer.cpp @@ -10,7 +10,12 @@ namespace Catch { - ITransientExpression::~ITransientExpression() = default; + void ITransientExpression::streamReconstructedExpression( + std::ostream& os ) const { + // We can't make this function pure virtual to keep ITransientExpression + // constexpr, so we write error message instead + os << "Some class derived from ITransientExpression without overriding streamReconstructedExpression"; + } void formatReconstructedExpression( std::ostream &os, std::string const& lhs, StringRef op, std::string const& rhs ) { if( lhs.size() + rhs.size() < 40 && diff --git a/src/catch2/internal/catch_decomposer.hpp b/src/catch2/internal/catch_decomposer.hpp index e0e46c1de8..d4c7b24371 100644 --- a/src/catch2/internal/catch_decomposer.hpp +++ b/src/catch2/internal/catch_decomposer.hpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -34,8 +35,33 @@ # pragma GCC diagnostic ignored "-Wsign-compare" #endif +#if defined(CATCH_CPP20_OR_GREATER) && __has_include() +# include +# if defined( __cpp_lib_three_way_comparison ) && \ + __cpp_lib_three_way_comparison >= 201907L +# define CATCH_CONFIG_CPP20_COMPARE_OVERLOADS +# endif +#endif + namespace Catch { + // Note: There is nothing that stops us from extending this, + // e.g. to `std::is_scalar`, but the more encompassing + // traits are usually also more expensive. For now we + // keep this as it used to be and it can be changed later. + template + struct capture_by_value + : std::integral_constant{}> {}; + +#if defined( CATCH_CONFIG_CPP20_COMPARE_OVERLOADS ) + template <> + struct capture_by_value : std::true_type {}; + template <> + struct capture_by_value : std::true_type {}; + template <> + struct capture_by_value : std::true_type {}; +#endif + template struct always_false : std::false_type {}; @@ -44,11 +70,12 @@ namespace Catch { bool m_result; public: - auto isBinaryExpression() const -> bool { return m_isBinaryExpression; } - auto getResult() const -> bool { return m_result; } - virtual void streamReconstructedExpression( std::ostream &os ) const = 0; + constexpr auto isBinaryExpression() const -> bool { return m_isBinaryExpression; } + constexpr auto getResult() const -> bool { return m_result; } + //! This function **has** to be overriden by the derived class. + virtual void streamReconstructedExpression( std::ostream& os ) const; - ITransientExpression( bool isBinaryExpression, bool result ) + constexpr ITransientExpression( bool isBinaryExpression, bool result ) : m_isBinaryExpression( isBinaryExpression ), m_result( result ) {} @@ -59,7 +86,7 @@ namespace Catch { // We don't actually need a virtual destructor, but many static analysers // complain if it's not here :-( - virtual ~ITransientExpression(); // = default; + virtual ~ITransientExpression() = default; friend std::ostream& operator<<(std::ostream& out, ITransientExpression const& expr) { expr.streamReconstructedExpression(out); @@ -81,7 +108,7 @@ namespace Catch { } public: - BinaryExpr( bool comparisonResult, LhsT lhs, StringRef op, RhsT rhs ) + constexpr BinaryExpr( bool comparisonResult, LhsT lhs, StringRef op, RhsT rhs ) : ITransientExpression{ true, comparisonResult }, m_lhs( lhs ), m_op( op ), @@ -154,7 +181,7 @@ namespace Catch { } public: - explicit UnaryExpr( LhsT lhs ) + explicit constexpr UnaryExpr( LhsT lhs ) : ITransientExpression{ false, static_cast(lhs) }, m_lhs( lhs ) {} @@ -165,30 +192,30 @@ namespace Catch { class ExprLhs { LhsT m_lhs; public: - explicit ExprLhs( LhsT lhs ) : m_lhs( lhs ) {} + explicit constexpr ExprLhs( LhsT lhs ) : m_lhs( lhs ) {} #define CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR( id, op ) \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ ->std::enable_if_t< \ Detail::conjunction, \ - Detail::negation>>>::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ ->std::enable_if_t< \ Detail::conjunction, \ - std::is_arithmetic>::value, \ + capture_by_value>::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ ->std::enable_if_t< \ Detail::conjunction< \ Detail::negation>, \ @@ -202,7 +229,7 @@ namespace Catch { static_cast( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ ->std::enable_if_t< \ Detail::conjunction< \ Detail::negation>, \ @@ -220,28 +247,29 @@ namespace Catch { #undef CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR + #define CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR( id, op ) \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ ->std::enable_if_t< \ Detail::conjunction, \ - Detail::negation>>>::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ ->std::enable_if_t< \ Detail::conjunction, \ - std::is_arithmetic>::value, \ + capture_by_value>::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ ->std::enable_if_t< \ Detail::conjunction< \ Detail::negation>, \ @@ -253,7 +281,7 @@ namespace Catch { static_cast( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ ->std::enable_if_t< \ Detail::conjunction< \ Detail::negation>, \ @@ -274,16 +302,16 @@ namespace Catch { #define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR( op ) \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ ->std::enable_if_t< \ - !std::is_arithmetic>::value, \ + !capture_by_value>::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ - friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->std::enable_if_t::value, \ + constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ + ->std::enable_if_t::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ @@ -309,19 +337,23 @@ namespace Catch { "wrap the expression inside parentheses, or decompose it"); } - auto makeUnaryExpr() const -> UnaryExpr { + constexpr auto makeUnaryExpr() const -> UnaryExpr { return UnaryExpr{ m_lhs }; } }; struct Decomposer { - template>::value, int> = 0> - friend auto operator <= ( Decomposer &&, T && lhs ) -> ExprLhs { + template >::value, + int> = 0> + constexpr friend auto operator <= ( Decomposer &&, T && lhs ) -> ExprLhs { return ExprLhs{ lhs }; } - template::value, int> = 0> - friend auto operator <= ( Decomposer &&, T value ) -> ExprLhs { + template ::value, int> = 0> + constexpr friend auto operator <= ( Decomposer &&, T value ) -> ExprLhs { return ExprLhs{ value }; } }; diff --git a/tests/SelfTest/UsageTests/Compilation.tests.cpp b/tests/SelfTest/UsageTests/Compilation.tests.cpp index 1cdcfb788e..46ccb8b73c 100644 --- a/tests/SelfTest/UsageTests/Compilation.tests.cpp +++ b/tests/SelfTest/UsageTests/Compilation.tests.cpp @@ -313,11 +313,12 @@ TEST_CASE("ADL universal operators don't hijack expression deconstruction", "[co REQUIRE(0 ^ adl::always_true{}); } -TEST_CASE( "#2555 - types that can only be compared with 0 literal (not int/long) are supported", "[compilation][approvals]" ) { +TEST_CASE( "#2555 - types that can only be compared with 0 literal implemented as pointer conversion are supported", + "[compilation][approvals]" ) { REQUIRE( TypeWithLit0Comparisons{} < 0 ); REQUIRE_FALSE( 0 < TypeWithLit0Comparisons{} ); REQUIRE( TypeWithLit0Comparisons{} <= 0 ); - REQUIRE_FALSE( 0 > TypeWithLit0Comparisons{} ); + REQUIRE_FALSE( 0 <= TypeWithLit0Comparisons{} ); REQUIRE( TypeWithLit0Comparisons{} > 0 ); REQUIRE_FALSE( 0 > TypeWithLit0Comparisons{} ); @@ -330,6 +331,72 @@ TEST_CASE( "#2555 - types that can only be compared with 0 literal (not int/long REQUIRE_FALSE( 0 != TypeWithLit0Comparisons{} ); } +// These tests require `consteval` to propagate through `constexpr` calls +// which is a late DR against C++20. +#if defined( CATCH_CPP20_OR_GREATER ) && defined( __cpp_consteval ) && \ + __cpp_consteval >= 202211L +// Can't have internal linkage to avoid warnings +void ZeroLiteralErrorFunc(); +namespace { + struct ZeroLiteralConsteval { + template , int> = 0> + consteval ZeroLiteralConsteval( T zero ) noexcept { + if ( zero != 0 ) { ZeroLiteralErrorFunc(); } + } + }; + + // Should only be constructible from literal 0. Uses the propagating + // consteval constructor trick (currently used by MSVC, might be used + // by libc++ in the future as well). + struct TypeWithConstevalLit0Comparison { +# define DEFINE_COMP_OP( op ) \ + constexpr friend bool operator op( TypeWithConstevalLit0Comparison, \ + ZeroLiteralConsteval ) { \ + return true; \ + } \ + constexpr friend bool operator op( ZeroLiteralConsteval, \ + TypeWithConstevalLit0Comparison ) { \ + return false; \ + } + + DEFINE_COMP_OP( < ) + DEFINE_COMP_OP( <= ) + DEFINE_COMP_OP( > ) + DEFINE_COMP_OP( >= ) + DEFINE_COMP_OP( == ) + DEFINE_COMP_OP( != ) + +#undef DEFINE_COMP_OP + }; + +} // namespace + +namespace Catch { + template <> + struct capture_by_value : std::true_type {}; +} + +TEST_CASE( "#2555 - types that can only be compared with 0 literal implemented as consteval check are supported", + "[compilation][approvals]" ) { + REQUIRE( TypeWithConstevalLit0Comparison{} < 0 ); + REQUIRE_FALSE( 0 < TypeWithConstevalLit0Comparison{} ); + REQUIRE( TypeWithConstevalLit0Comparison{} <= 0 ); + REQUIRE_FALSE( 0 <= TypeWithConstevalLit0Comparison{} ); + + REQUIRE( TypeWithConstevalLit0Comparison{} > 0 ); + REQUIRE_FALSE( 0 > TypeWithConstevalLit0Comparison{} ); + REQUIRE( TypeWithConstevalLit0Comparison{} >= 0 ); + REQUIRE_FALSE( 0 >= TypeWithConstevalLit0Comparison{} ); + + REQUIRE( TypeWithConstevalLit0Comparison{} == 0 ); + REQUIRE_FALSE( 0 == TypeWithConstevalLit0Comparison{} ); + REQUIRE( TypeWithConstevalLit0Comparison{} != 0 ); + REQUIRE_FALSE( 0 != TypeWithConstevalLit0Comparison{} ); +} + +#endif // C++20 consteval + + namespace { struct MultipleImplicitConstructors { MultipleImplicitConstructors( double ) {} diff --git a/tests/SelfTest/helpers/type_with_lit_0_comparisons.hpp b/tests/SelfTest/helpers/type_with_lit_0_comparisons.hpp index 202c3af4d9..88d2f2a74d 100644 --- a/tests/SelfTest/helpers/type_with_lit_0_comparisons.hpp +++ b/tests/SelfTest/helpers/type_with_lit_0_comparisons.hpp @@ -12,23 +12,28 @@ #include // Should only be constructible from literal 0. +// Based on the constructor from pointer trick, used by libstdc++ and libc++ +// (formerly also MSVC, but they've moved to consteval int constructor). // Used by `TypeWithLit0Comparisons` for testing comparison // ops that only work with literal zero, the way std::*orderings do -struct ZeroLiteralDetector { - constexpr ZeroLiteralDetector( ZeroLiteralDetector* ) noexcept {} +struct ZeroLiteralAsPointer { + constexpr ZeroLiteralAsPointer( ZeroLiteralAsPointer* ) noexcept {} template ::value>> - constexpr ZeroLiteralDetector( T ) = delete; + constexpr ZeroLiteralAsPointer( T ) = delete; }; + struct TypeWithLit0Comparisons { -#define DEFINE_COMP_OP( op ) \ - friend bool operator op( TypeWithLit0Comparisons, ZeroLiteralDetector ) { \ - return true; \ - } \ - friend bool operator op( ZeroLiteralDetector, TypeWithLit0Comparisons ) { \ - return false; \ +#define DEFINE_COMP_OP( op ) \ + constexpr friend bool operator op( TypeWithLit0Comparisons, \ + ZeroLiteralAsPointer ) { \ + return true; \ + } \ + constexpr friend bool operator op( ZeroLiteralAsPointer, \ + TypeWithLit0Comparisons ) { \ + return false; \ } DEFINE_COMP_OP( < )