From 71b11c4e3328090d2ec72268299e3fb9e0ac3f8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Mon, 8 Apr 2024 11:02:22 +0200 Subject: [PATCH] Fix assertion for const instance of std::ordering The issue was that `capture_by_value` was meant to be specialized by plain type, e.g. `capture_by_value`, but the TMP in Decomposer did not properly throw away `const` (and `volatile`) qualifiers, before taking the value of `capture_by_value`. --- src/catch2/internal/catch_decomposer.hpp | 37 +++++++++++-------- .../SelfTest/UsageTests/Compilation.tests.cpp | 35 ++++++++++++++++++ 2 files changed, 56 insertions(+), 16 deletions(-) diff --git a/src/catch2/internal/catch_decomposer.hpp b/src/catch2/internal/catch_decomposer.hpp index 143ee3d3..41365b70 100644 --- a/src/catch2/internal/catch_decomposer.hpp +++ b/src/catch2/internal/catch_decomposer.hpp @@ -125,6 +125,12 @@ namespace Catch { + namespace Detail { + // This was added in C++20, but we require only C++14 for now. + template + using RemoveCVRef_t = std::remove_cv_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 @@ -276,17 +282,17 @@ namespace Catch { #define CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR( id, op ) \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ - ->std::enable_if_t< \ + -> std::enable_if_t< \ Detail::conjunction, \ Detail::negation>>>::value, \ + Detail::RemoveCVRef_t>>>::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->std::enable_if_t< \ + -> std::enable_if_t< \ Detail::conjunction, \ capture_by_value>::value, \ BinaryExpr> { \ @@ -295,7 +301,7 @@ namespace Catch { } \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->std::enable_if_t< \ + -> std::enable_if_t< \ Detail::conjunction< \ Detail::negation>, \ Detail::is_eq_0_comparable, \ @@ -309,7 +315,7 @@ namespace Catch { } \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->std::enable_if_t< \ + -> std::enable_if_t< \ Detail::conjunction< \ Detail::negation>, \ Detail::is_eq_0_comparable, \ @@ -330,17 +336,17 @@ namespace Catch { #define CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR( id, op ) \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ - ->std::enable_if_t< \ + -> std::enable_if_t< \ Detail::conjunction, \ Detail::negation>>>::value, \ + Detail::RemoveCVRef_t>>>::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->std::enable_if_t< \ + -> std::enable_if_t< \ Detail::conjunction, \ capture_by_value>::value, \ BinaryExpr> { \ @@ -349,7 +355,7 @@ namespace Catch { } \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->std::enable_if_t< \ + -> std::enable_if_t< \ Detail::conjunction< \ Detail::negation>, \ Detail::is_##id##_0_comparable, \ @@ -361,7 +367,7 @@ namespace Catch { } \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->std::enable_if_t< \ + -> std::enable_if_t< \ Detail::conjunction< \ Detail::negation>, \ Detail::is_##id##_0_comparable, \ @@ -382,16 +388,16 @@ namespace Catch { #define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR( op ) \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \ - ->std::enable_if_t< \ - !capture_by_value>::value, \ + -> std::enable_if_t< \ + !capture_by_value>::value, \ BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } \ template \ constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \ - ->std::enable_if_t::value, \ - BinaryExpr> { \ + -> std::enable_if_t::value, \ + BinaryExpr> { \ return { \ static_cast( lhs.m_lhs op rhs ), lhs.m_lhs, #op##_sr, rhs }; \ } @@ -423,8 +429,7 @@ namespace Catch { struct Decomposer { template >::value, + std::enable_if_t>::value, int> = 0> constexpr friend auto operator <= ( Decomposer &&, T && lhs ) -> ExprLhs { return ExprLhs{ lhs }; diff --git a/tests/SelfTest/UsageTests/Compilation.tests.cpp b/tests/SelfTest/UsageTests/Compilation.tests.cpp index 46ccb8b7..ebb2add6 100644 --- a/tests/SelfTest/UsageTests/Compilation.tests.cpp +++ b/tests/SelfTest/UsageTests/Compilation.tests.cpp @@ -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( < ) @@ -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 @@ -420,3 +441,17 @@ 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; + REQUIRE( const_ordering_1 == plain_ordering_1 ); + REQUIRE( const_ordering_1 == const_ordering_2 ); + REQUIRE( plain_ordering_1 == const_ordering_1 ); +} +#endif