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<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`.

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 <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
@@ -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>> {                                          \
@@ -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>,                              \
@@ -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>,                              \
@@ -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>> {                                          \
@@ -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>,                          \
@@ -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>,                          \
@@ -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 }; \
     }
@@ -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 };
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