Skip to content

Commit

Permalink
Move SFINAE in decomposer into return type
Browse files Browse the repository at this point in the history 
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> == int`
and `int == ExprLhs<int>`. Since the overload set and SFINAE
constraints assume that `ExprLhs<T>` 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
  • Loading branch information
@horenmar
horenmar committed Dec 8, 2022
1 parent 2d7be1f commit 28e651f
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 82 deletions.
10 changes: 10 additions & 0 deletions src/catch2/internal/catch_compare_traits.hpp
View file
Expand Up @@ -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 <typename, typename, typename = void> \
struct is_##id##_comparable : std::false_type {}; \
Expand Down Expand Up @@ -56,6 +63,9 @@ namespace Catch {

#if defined( __GNUC__ ) && !defined( __clang__ )
# pragma GCC diagnostic pop
#endif
#if defined( __clang__ )
# pragma clang diagnostic pop
#endif


Expand Down
159 changes: 77 additions & 82 deletions src/catch2/internal/catch_decomposer.hpp
View file
Expand Up @@ -13,6 +13,7 @@
#include <catch2/internal/catch_meta.hpp>
#include <catch2/internal/catch_compare_traits.hpp>
#include <catch2/internal/catch_test_failure_exception.hpp>
#include <catch2/internal/catch_logical_traits.hpp>

#include <type_traits>
#include <iosfwd>
Expand Down Expand Up @@ -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<LhsT, RhsT>::value && \
!std::is_arithmetic<std::remove_reference_t<RhsT>>::value, \
int> = 0> \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->BinaryExpr<LhsT, RhsT const&> { \
->std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
Detail::negation<std::is_arithmetic< \
std::remove_reference_t<RhsT>>>>::value, \
BinaryExpr<LhsT, RhsT const&>> { \
return { \
static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template < \
typename RhsT, \
std::enable_if_t<Detail::is_##id##_comparable<LhsT, RhsT>::value && \
std::is_arithmetic<RhsT>::value, \
int> = 0> \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->BinaryExpr<LhsT, RhsT> { \
->std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
std::is_arithmetic<RhsT>>::value, \
BinaryExpr<LhsT, RhsT>> { \
return { \
static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template < \
typename RhsT, \
std::enable_if_t<!Detail::is_##id##_comparable<LhsT, RhsT>::value && \
Detail::is_eq_0_comparable<LhsT>:: \
value && /* We allow long because we want \
`ptr op NULL to be accepted */ \
( std::is_same<RhsT, int>::value || \
std::is_same<RhsT, long>::value ), \
int> = 0> \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->BinaryExpr<LhsT, RhsT> { \
if ( rhs != 0 ) { \
throw_test_failure_exception(); \
} \
->std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Detail::is_eq_0_comparable<LhsT>, \
/* We allow long because we want `ptr op NULL` to be accepted */ \
Detail::disjunction<std::is_same<RhsT, int>, \
std::is_same<RhsT, long>>>::value, \
BinaryExpr<LhsT, RhsT>> { \
if ( rhs != 0 ) { throw_test_failure_exception(); } \
return { \
static_cast<bool>( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template < \
typename RhsT, \
std::enable_if_t<!Detail::is_##id##_comparable<LhsT, RhsT>::value && \
Detail::is_eq_0_comparable<RhsT>:: \
value && /* We allow long because we want \
`ptr op NULL` to be accepted */ \
( std::is_same<LhsT, int>::value || \
std::is_same<LhsT, long>::value ), \
int> = 0> \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->BinaryExpr<LhsT, RhsT> { \
if ( lhs.m_lhs != 0 ) { \
throw_test_failure_exception(); \
} \
->std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Detail::is_eq_0_comparable<RhsT>, \
/* We allow long because we want `ptr op NULL` to be accepted */ \
Detail::disjunction<std::is_same<LhsT, int>, \
std::is_same<LhsT, long>>>::value, \
BinaryExpr<LhsT, RhsT>> { \
if ( lhs.m_lhs != 0 ) { throw_test_failure_exception(); } \
return { static_cast<bool>( 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<LhsT, RhsT>::value && \
!std::is_arithmetic<std::remove_reference_t<RhsT>>::value, \
int> = 0> \
#define CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR( id, op ) \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->BinaryExpr<LhsT, RhsT const&> { \
->std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
Detail::negation<std::is_arithmetic< \
std::remove_reference_t<RhsT>>>>::value, \
BinaryExpr<LhsT, RhsT const&>> { \
return { \
static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template < \
typename RhsT, \
std::enable_if_t<Detail::is_##id##_comparable<LhsT, RhsT>::value && \
std::is_arithmetic<RhsT>::value, \
int> = 0> \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->BinaryExpr<LhsT, RhsT> { \
->std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
std::is_arithmetic<RhsT>>::value, \
BinaryExpr<LhsT, RhsT>> { \
return { \
static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template < \
typename RhsT, \
std::enable_if_t<!Detail::is_##id##_comparable<LhsT, RhsT>::value && \
Detail::is_##id##_0_comparable<LhsT>::value && \
std::is_same<RhsT, int>::value, \
int> = 0> \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->BinaryExpr<LhsT, RhsT> { \
if ( rhs != 0 ) { \
throw_test_failure_exception(); \
} \
->std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Detail::is_##id##_0_comparable<LhsT>, \
std::is_same<RhsT, int>>::value, \
BinaryExpr<LhsT, RhsT>> { \
if ( rhs != 0 ) { throw_test_failure_exception(); } \
return { \
static_cast<bool>( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template < \
typename RhsT, \
std::enable_if_t<!Detail::is_##id##_comparable<LhsT, RhsT>::value && \
Detail::is_##id##_0_comparable<RhsT>::value && \
std::is_same<LhsT, int>::value, \
int> = 0> \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->BinaryExpr<LhsT, RhsT> { \
if ( lhs.m_lhs != 0 ) { \
throw_test_failure_exception(); \
} \
->std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Detail::is_##id##_0_comparable<RhsT>, \
std::is_same<LhsT, int>>::value, \
BinaryExpr<LhsT, RhsT>> { \
if ( lhs.m_lhs != 0 ) { throw_test_failure_exception(); } \
return { static_cast<bool>( 0 op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
}

Expand All @@ -282,15 +270,22 @@ namespace Catch {
#undef CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR


#define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR(op) \
template<typename RhsT, std::enable_if_t<!std::is_arithmetic<std::remove_reference_t<RhsT>>::value, int> = 0> \
friend auto operator op ( ExprLhs && lhs, RhsT && rhs ) -> BinaryExpr<LhsT, RhsT const&> { \
return { static_cast<bool>(lhs.m_lhs op rhs), lhs.m_lhs, #op##_sr, rhs }; \
} \
template<typename RhsT, std::enable_if_t<std::is_arithmetic<RhsT>::value, int> = 0> \
friend auto operator op ( ExprLhs && lhs, RhsT rhs ) -> BinaryExpr<LhsT, RhsT> { \
return { static_cast<bool>(lhs.m_lhs op rhs), lhs.m_lhs, #op##_sr, rhs }; \
}
#define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR( op ) \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->std::enable_if_t< \
!std::is_arithmetic<std::remove_reference_t<RhsT>>::value, \
BinaryExpr<LhsT, RhsT const&>> { \
return { \
static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t<std::is_arithmetic<RhsT>::value, \
BinaryExpr<LhsT, RhsT>> { \
return { \
static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
}

CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR(|)
CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR(&)
Expand Down
24 changes: 24 additions & 0 deletions tests/SelfTest/UsageTests/Compilation.tests.cpp
View file
Expand Up @@ -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 );
}

0 comments on commit 28e651f

Please sign in to comment.