Skip to content

Commit

Permalink
Fix assertion for const instance of std::ordering
Browse files Browse the repository at this point in the history 
The issue was that `capture_by_value` was meant to be specialized
by plain type, e.g. `capture_by_value<std::weak_ordering>`, but
the TMP in Decomposer did not properly throw away `const` (and
`volatile`) qualifiers, before taking the value of `capture_by_value`.
  • Loading branch information
@horenmar
horenmar committed Apr 8, 2024
1 parent 0a6a2ce commit b2c3658
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 16 deletions.
37 changes: 21 additions & 16 deletions src/catch2/internal/catch_decomposer.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,12 @@

namespace Catch {

namespace Detail {
// This was added in C++20, but we require only C++14 for now.
template <typename T>
using RemoveCVRef_t = std::remove_cv_t<std::remove_reference_t<T>>;
}

// 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
Expand Down Expand Up @@ -276,17 +282,17 @@ namespace Catch {
#define CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR( id, op ) \
template <typename RhsT> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->std::enable_if_t< \
-> std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
Detail::negation<capture_by_value< \
std::remove_reference_t<RhsT>>>>::value, \
Detail::RemoveCVRef_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> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
-> std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
capture_by_value<RhsT>>::value, \
BinaryExpr<LhsT, RhsT>> { \
Expand All @@ -295,7 +301,7 @@ namespace Catch {
} \
template <typename RhsT> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
-> std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Detail::is_eq_0_comparable<LhsT>, \
Expand All @@ -309,7 +315,7 @@ namespace Catch {
} \
template <typename RhsT> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
-> std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Detail::is_eq_0_comparable<RhsT>, \
Expand All @@ -330,17 +336,17 @@ namespace Catch {
#define CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR( id, op ) \
template <typename RhsT> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->std::enable_if_t< \
-> std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
Detail::negation<capture_by_value< \
std::remove_reference_t<RhsT>>>>::value, \
Detail::RemoveCVRef_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> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
-> std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
capture_by_value<RhsT>>::value, \
BinaryExpr<LhsT, RhsT>> { \
Expand All @@ -349,7 +355,7 @@ namespace Catch {
} \
template <typename RhsT> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
-> std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Detail::is_##id##_0_comparable<LhsT>, \
Expand All @@ -361,7 +367,7 @@ namespace Catch {
} \
template <typename RhsT> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
-> std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Detail::is_##id##_0_comparable<RhsT>, \
Expand All @@ -382,16 +388,16 @@ namespace Catch {
#define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR( op ) \
template <typename RhsT> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->std::enable_if_t< \
!capture_by_value<std::remove_reference_t<RhsT>>::value, \
-> std::enable_if_t< \
!capture_by_value<Detail::RemoveCVRef_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> \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t<capture_by_value<RhsT>::value, \
BinaryExpr<LhsT, RhsT>> { \
-> std::enable_if_t<capture_by_value<RhsT>::value, \
BinaryExpr<LhsT, RhsT>> { \
return { \
static_cast<bool>( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \
}
Expand Down Expand Up @@ -423,8 +429,7 @@ namespace Catch {

struct Decomposer {
template <typename T,
std::enable_if_t<
!capture_by_value<std::remove_reference_t<T>>::value,
std::enable_if_t<!capture_by_value<Detail::RemoveCVRef_t<T>>::value,
int> = 0>
constexpr friend auto operator <= ( Decomposer &&, T && lhs ) -> ExprLhs<T const&> {
return ExprLhs<const T&>{ lhs };
Expand Down
36 changes: 36 additions & 0 deletions tests/SelfTest/UsageTests/Compilation.tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,12 @@ namespace {
constexpr friend bool operator op( ZeroLiteralConsteval, \
TypeWithConstevalLit0Comparison ) { \
return false; \
} \
/* std::orderings only have these for ==, but we add them for all \
operators so we can test all overloads for decomposer */ \
constexpr friend bool operator op( TypeWithConstevalLit0Comparison, \
TypeWithConstevalLit0Comparison ) { \
return true; \
}

DEFINE_COMP_OP( < )
Expand Down Expand Up @@ -394,6 +400,21 @@ TEST_CASE( "#2555 - types that can only be compared with 0 literal implemented a
REQUIRE_FALSE( 0 != TypeWithConstevalLit0Comparison{} );
}

// We check all comparison ops to test, even though orderings, the primary
// motivation for this functionality, only have self-comparison (and thus
// have the ambiguity issue) for `==` and `!=`.
TEST_CASE( "Comparing const instances of type registered with capture_by_value",
"[regression][approvals][compilation]" ) {
auto const const_Lit0Type_1 = TypeWithLit0Comparisons{};
auto const const_Lit0Type_2 = TypeWithLit0Comparisons{};
REQUIRE( const_Lit0Type_1 == const_Lit0Type_2 );
REQUIRE( const_Lit0Type_1 <= const_Lit0Type_2 );
REQUIRE( const_Lit0Type_1 < const_Lit0Type_2 );
REQUIRE( const_Lit0Type_1 >= const_Lit0Type_2 );
REQUIRE( const_Lit0Type_1 > const_Lit0Type_2 );
REQUIRE_FALSE( const_Lit0Type_1 != const_Lit0Type_2 );
}

#endif // C++20 consteval


Expand All @@ -420,3 +441,18 @@ TEST_CASE("#2571 - tests compile types that have multiple implicit constructors
REQUIRE( mic1 > mic2 );
REQUIRE( mic1 >= mic2 );
}

#if defined( CATCH_CONFIG_CPP20_COMPARE_OVERLOADS )
// This test does not test all the related codepaths, but it is the original
// reproducer
TEST_CASE( "Comparing const std::weak_ordering instances must compile",
"[compilation][approvals][regression]" ) {
auto const const_ordering_1 = std::weak_ordering::less;
auto const const_ordering_2 = std::weak_ordering::less;
auto plain_ordering_1 = std::weak_ordering::less;
auto plain_ordering_2 = std::weak_ordering::less;
REQUIRE( const_ordering_1 == plain_ordering_1 );
REQUIRE( const_ordering_1 == const_ordering_2 );
REQUIRE( plain_ordering_1 == const_ordering_1 );
}
#endif

0 comments on commit b2c3658

Please sign in to comment.