From 28e651f1521449f2fce8b202307b6312a0ebe429 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Thu, 8 Dec 2022 16:31:55 +0100 Subject: [PATCH] Move SFINAE in decomposer into return type This is needed so that we can use conjunction and other logical type traits to workaround issue with older GCC versions (8 and below), when they run into types that have ambiguous constructor from `0`, see e.g. #2571. However, using conjunction and friends in the SFINAE constraint in the template parameter breaks for C++20 and up, due to the new comparison operator rewriting rules. With C++20, when the compiler see `a == b`, it also tries `b == a` and collects overload set for both of these expressions. In Catch2, this means that e.g. `REQUIRE( 1 == 2 )` would lead the compiler to check overloads for both `ExprLhs == int` and `int == ExprLhs`. Since the overload set and SFINAE constraints assume that `ExprLhs` is always on the left side, when the compiler tries to resolve the template parameters, all hell breaks loose and the compilation fails. By moving the SFINAE constraints to the return type, the compiler can discard the switched expression without having to resolve the complex SFINAE constraints, and thus everything works the way it is supposed to. Fixes #2571 --- src/catch2/internal/catch_compare_traits.hpp | 10 ++ src/catch2/internal/catch_decomposer.hpp | 159 +++++++++--------- .../SelfTest/UsageTests/Compilation.tests.cpp | 24 +++ 3 files changed, 111 insertions(+), 82 deletions(-) diff --git a/src/catch2/internal/catch_compare_traits.hpp b/src/catch2/internal/catch_compare_traits.hpp index 2debeced..6304b1ff 100644 --- a/src/catch2/internal/catch_compare_traits.hpp +++ b/src/catch2/internal/catch_compare_traits.hpp @@ -28,6 +28,13 @@ namespace Catch { # pragma GCC diagnostic ignored "-Wfloat-equal" #endif +#if defined( __clang__ ) +# pragma clang diagnostic push + // Did you know that comparing floats with `0` directly + // is super-duper dangerous in unevaluated context? +# pragma clang diagnostic ignored "-Wfloat-equal" +#endif + #define CATCH_DEFINE_COMPARABLE_TRAIT( id, op ) \ template \ struct is_##id##_comparable : std::false_type {}; \ @@ -56,6 +63,9 @@ namespace Catch { #if defined( __GNUC__ ) && !defined( __clang__ ) # pragma GCC diagnostic pop +#endif +#if defined( __clang__ ) +# pragma clang diagnostic pop #endif diff --git a/src/catch2/internal/catch_decomposer.hpp b/src/catch2/internal/catch_decomposer.hpp index 3448803f..792dd330 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 @@ -165,112 +166,99 @@ namespace Catch { explicit ExprLhs( LhsT lhs ) : m_lhs( lhs ) {} #define CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR( id, op ) \ - template < \ - typename RhsT, \ - std::enable_if_t< \ - Detail::is_##id##_comparable::value && \ - !std::is_arithmetic>::value, \ - int> = 0> \ + template \ friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ - ->BinaryExpr { \ + ->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 < \ - typename RhsT, \ - std::enable_if_t::value && \ - std::is_arithmetic::value, \ - int> = 0> \ + template \ friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->BinaryExpr { \ + ->std::enable_if_t< \ + Detail::conjunction, \ + std::is_arithmetic>::value, \ + BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ - template < \ - typename RhsT, \ - std::enable_if_t::value && \ - Detail::is_eq_0_comparable:: \ - value && /* We allow long because we want \ - `ptr op NULL to be accepted */ \ - ( std::is_same::value || \ - std::is_same::value ), \ - int> = 0> \ + template \ friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->BinaryExpr { \ - if ( rhs != 0 ) { \ - throw_test_failure_exception(); \ - } \ + ->std::enable_if_t< \ + Detail::conjunction< \ + Detail::negation>, \ + Detail::is_eq_0_comparable, \ + /* We allow long because we want `ptr op NULL` to be accepted */ \ + Detail::disjunction, \ + std::is_same>>::value, \ + BinaryExpr> { \ + if ( rhs != 0 ) { throw_test_failure_exception(); } \ return { \ static_cast( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs }; \ } \ - template < \ - typename RhsT, \ - std::enable_if_t::value && \ - Detail::is_eq_0_comparable:: \ - value && /* We allow long because we want \ - `ptr op NULL` to be accepted */ \ - ( std::is_same::value || \ - std::is_same::value ), \ - int> = 0> \ + template \ friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->BinaryExpr { \ - if ( lhs.m_lhs != 0 ) { \ - throw_test_failure_exception(); \ - } \ + ->std::enable_if_t< \ + Detail::conjunction< \ + Detail::negation>, \ + Detail::is_eq_0_comparable, \ + /* We allow long because we want `ptr op NULL` to be accepted */ \ + Detail::disjunction, \ + std::is_same>>::value, \ + BinaryExpr> { \ + if ( lhs.m_lhs != 0 ) { throw_test_failure_exception(); } \ return { static_cast( 0 op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } + CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR( eq, == ) CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR( ne, != ) #undef CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR - #define CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR( id, op ) \ - template < \ - typename RhsT, \ - std::enable_if_t< \ - Detail::is_##id##_comparable::value && \ - !std::is_arithmetic>::value, \ - int> = 0> \ +#define CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR( id, op ) \ + template \ friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ - ->BinaryExpr { \ + ->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 < \ - typename RhsT, \ - std::enable_if_t::value && \ - std::is_arithmetic::value, \ - int> = 0> \ + template \ friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->BinaryExpr { \ + ->std::enable_if_t< \ + Detail::conjunction, \ + std::is_arithmetic>::value, \ + BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ - template < \ - typename RhsT, \ - std::enable_if_t::value && \ - Detail::is_##id##_0_comparable::value && \ - std::is_same::value, \ - int> = 0> \ + template \ friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->BinaryExpr { \ - if ( rhs != 0 ) { \ - throw_test_failure_exception(); \ - } \ + ->std::enable_if_t< \ + Detail::conjunction< \ + Detail::negation>, \ + Detail::is_##id##_0_comparable, \ + std::is_same>::value, \ + BinaryExpr> { \ + if ( rhs != 0 ) { throw_test_failure_exception(); } \ return { \ static_cast( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs }; \ } \ - template < \ - typename RhsT, \ - std::enable_if_t::value && \ - Detail::is_##id##_0_comparable::value && \ - std::is_same::value, \ - int> = 0> \ + template \ friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->BinaryExpr { \ - if ( lhs.m_lhs != 0 ) { \ - throw_test_failure_exception(); \ - } \ + ->std::enable_if_t< \ + Detail::conjunction< \ + Detail::negation>, \ + Detail::is_##id##_0_comparable, \ + std::is_same>::value, \ + BinaryExpr> { \ + if ( lhs.m_lhs != 0 ) { throw_test_failure_exception(); } \ return { static_cast( 0 op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } @@ -282,15 +270,22 @@ namespace Catch { #undef CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR - #define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR(op) \ - template>::value, int> = 0> \ - friend auto operator op ( ExprLhs && lhs, RhsT && rhs ) -> BinaryExpr { \ - return { static_cast(lhs.m_lhs op rhs), lhs.m_lhs, #op##_sr, rhs }; \ - } \ - template::value, int> = 0> \ - friend auto operator op ( ExprLhs && lhs, RhsT rhs ) -> BinaryExpr { \ - return { static_cast(lhs.m_lhs op rhs), lhs.m_lhs, #op##_sr, rhs }; \ - } +#define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR( op ) \ + template \ + friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ + ->std::enable_if_t< \ + !std::is_arithmetic>::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, \ + BinaryExpr> { \ + return { \ + static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ + } CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR(|) CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR(&) diff --git a/tests/SelfTest/UsageTests/Compilation.tests.cpp b/tests/SelfTest/UsageTests/Compilation.tests.cpp index a61a9674..1cdcfb78 100644 --- a/tests/SelfTest/UsageTests/Compilation.tests.cpp +++ b/tests/SelfTest/UsageTests/Compilation.tests.cpp @@ -329,3 +329,27 @@ TEST_CASE( "#2555 - types that can only be compared with 0 literal (not int/long REQUIRE( TypeWithLit0Comparisons{} != 0 ); REQUIRE_FALSE( 0 != TypeWithLit0Comparisons{} ); } + +namespace { + struct MultipleImplicitConstructors { + MultipleImplicitConstructors( double ) {} + MultipleImplicitConstructors( int64_t ) {} + bool operator==( MultipleImplicitConstructors ) const { return true; } + bool operator!=( MultipleImplicitConstructors ) const { return true; } + bool operator<( MultipleImplicitConstructors ) const { return true; } + bool operator<=( MultipleImplicitConstructors ) const { return true; } + bool operator>( MultipleImplicitConstructors ) const { return true; } + bool operator>=( MultipleImplicitConstructors ) const { return true; } + }; +} +TEST_CASE("#2571 - tests compile types that have multiple implicit constructors from lit 0", + "[compilation][approvals]") { + MultipleImplicitConstructors mic1( 0.0 ); + MultipleImplicitConstructors mic2( 0.0 ); + REQUIRE( mic1 == mic2 ); + REQUIRE( mic1 != mic2 ); + REQUIRE( mic1 < mic2 ); + REQUIRE( mic1 <= mic2 ); + REQUIRE( mic1 > mic2 ); + REQUIRE( mic1 >= mic2 ); +}