Skip to content

Commit

Permalink
Support literal-zero detectors using consteval int constructors
Browse files Browse the repository at this point in the history 
This was originally motivated by `REQUIRE((a <=> b) == 0)` no
longer compiling using MSVC. After some investigation, I found
that they changed their implementation of the zero literal
detector from the previous pointer-constructor with deleted
other constructors, into one that uses `consteval` constructor
from int.

This breaks the previous detection logic, because now
`is_foo_comparable<std::strong_ordering, int>` is true, but
actually trying to compare them is a compile-time error...
The solution was to make the decomposition `constexpr` and rely
on a late C++20 DR that makes it so that `consteval` propagates
up through the callstack of `constexpr` functions, until it either
runs out of `constexpr` functions, or succeeds.

However, the default handling of types in decomposition is to
take a reference to them. This reference never becomes dangling,
but because the constexpr evaluation engine cannot prove this,
decomposition paths taking references to objects cannot be
actually evaluated at compilation time. Thankfully we already
did have a value-oriented decomposition path for arithmetic types
(as these are common linkage-less types), so we could just
explicitly spell out the `std::foo_ordering` types as also being
supposed to be decomposed by-value.

Two more fun facts about these changes
 1) The original motivation of the MSVC change was to avoid
    trigering a `Wzero-as-null-pointer-constant` warning. I still
    do not believe this was a good decision.
 2) Current latest version of MSVC does not actually implement the
    aforementioned C++20 DR, so even with this commit, MSVC cannot
    compile `REQUIRE((a <=> b) == 0)`.
  • Loading branch information
@horenmar
horenmar committed Feb 11, 2024
1 parent bbba3d8 commit dc51386
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 41 deletions.
4 changes: 4 additions & 0 deletions src/catch2/internal/catch_compiler_capabilities.hpp
View file
Expand Up @@ -37,6 +37,10 @@
# define CATCH_CPP17_OR_GREATER
# endif

# if (__cplusplus >= 202002L) || (defined(_MSVC_LANG) && _MSVC_LANG >= 202002L)
# define CATCH_CPP20_OR_GREATER
# endif

#endif

// Only GCC compiler should be used in this block, so other compilers trying to
Expand Down
7 changes: 6 additions & 1 deletion src/catch2/internal/catch_decomposer.cpp
View file
Expand Up @@ -10,7 +10,12 @@

namespace Catch {

ITransientExpression::~ITransientExpression() = default;
void ITransientExpression::streamReconstructedExpression(
std::ostream& os ) const {
// We can't make this function pure virtual to keep ITransientExpression
// constexpr, so we write error message instead
os << "Some class derived from ITransientExpression without overriding streamReconstructedExpression";
}

void formatReconstructedExpression( std::ostream &os, std::string const& lhs, StringRef op, std::string const& rhs ) {
if( lhs.size() + rhs.size() < 40 &&
Expand Down
90 changes: 61 additions & 29 deletions src/catch2/internal/catch_decomposer.hpp
View file
Expand Up @@ -13,6 +13,7 @@
#include <catch2/internal/catch_compare_traits.hpp>
#include <catch2/internal/catch_test_failure_exception.hpp>
#include <catch2/internal/catch_logical_traits.hpp>
#include <catch2/internal/catch_compiler_capabilities.hpp>

#include <type_traits>
#include <iosfwd>
Expand All @@ -34,8 +35,33 @@
# pragma GCC diagnostic ignored "-Wsign-compare"
#endif

#if defined(CATCH_CPP20_OR_GREATER) && __has_include(<compare>)
# include <compare>
# if defined( __cpp_lib_three_way_comparison ) && \
__cpp_lib_three_way_comparison >= 201907L
# define CATCH_CONFIG_CPP20_COMPARE_OVERLOADS
# endif
#endif

namespace Catch {

// 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
// keep this as it used to be and it can be changed later.
template <typename T>
struct capture_by_value
: std::integral_constant<bool, std::is_arithmetic<T>{}> {};

#if defined( CATCH_CONFIG_CPP20_COMPARE_OVERLOADS )
template <>
struct capture_by_value<std::strong_ordering> : std::true_type {};
template <>
struct capture_by_value<std::weak_ordering> : std::true_type {};
template <>
struct capture_by_value<std::partial_ordering> : std::true_type {};
#endif

template <typename T>
struct always_false : std::false_type {};

Expand All @@ -44,11 +70,12 @@ namespace Catch {
bool m_result;

public:
auto isBinaryExpression() const -> bool { return m_isBinaryExpression; }
auto getResult() const -> bool { return m_result; }
virtual void streamReconstructedExpression( std::ostream &os ) const = 0;
constexpr auto isBinaryExpression() const -> bool { return m_isBinaryExpression; }
constexpr auto getResult() const -> bool { return m_result; }
//! This function **has** to be overriden by the derived class.
virtual void streamReconstructedExpression( std::ostream& os ) const;

ITransientExpression( bool isBinaryExpression, bool result )
constexpr ITransientExpression( bool isBinaryExpression, bool result )
: m_isBinaryExpression( isBinaryExpression ),
m_result( result )
{}
Expand All @@ -59,7 +86,7 @@ namespace Catch {

// We don't actually need a virtual destructor, but many static analysers
// complain if it's not here :-(
virtual ~ITransientExpression(); // = default;
virtual ~ITransientExpression() = default;

friend std::ostream& operator<<(std::ostream& out, ITransientExpression const& expr) {
expr.streamReconstructedExpression(out);
Expand All @@ -81,7 +108,7 @@ namespace Catch {
}

public:
BinaryExpr( bool comparisonResult, LhsT lhs, StringRef op, RhsT rhs )
constexpr BinaryExpr( bool comparisonResult, LhsT lhs, StringRef op, RhsT rhs )
: ITransientExpression{ true, comparisonResult },
m_lhs( lhs ),
m_op( op ),
Expand Down Expand Up @@ -154,7 +181,7 @@ namespace Catch {
}

public:
explicit UnaryExpr( LhsT lhs )
explicit constexpr UnaryExpr( LhsT lhs )
: ITransientExpression{ false, static_cast<bool>(lhs) },
m_lhs( lhs )
{}
Expand All @@ -165,30 +192,30 @@ namespace Catch {
class ExprLhs {
LhsT m_lhs;
public:
explicit ExprLhs( LhsT lhs ) : m_lhs( lhs ) {}
explicit constexpr ExprLhs( LhsT lhs ) : m_lhs( lhs ) {}

#define CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR( id, op ) \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
Detail::negation<std::is_arithmetic< \
Detail::negation<capture_by_value< \
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 ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
std::is_arithmetic<RhsT>>::value, \
capture_by_value<RhsT>>::value, \
BinaryExpr<LhsT, RhsT>> { \
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 ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Expand All @@ -202,7 +229,7 @@ namespace Catch {
static_cast<bool>( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Expand All @@ -220,28 +247,29 @@ namespace Catch {

#undef CATCH_INTERNAL_DEFINE_EXPRESSION_EQUALITY_OPERATOR


#define CATCH_INTERNAL_DEFINE_EXPRESSION_COMPARISON_OPERATOR( id, op ) \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
Detail::negation<std::is_arithmetic< \
Detail::negation<capture_by_value< \
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 ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
Detail::conjunction<Detail::is_##id##_comparable<LhsT, RhsT>, \
std::is_arithmetic<RhsT>>::value, \
capture_by_value<RhsT>>::value, \
BinaryExpr<LhsT, RhsT>> { \
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 ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Expand All @@ -253,7 +281,7 @@ namespace Catch {
static_cast<bool>( lhs.m_lhs op 0 ), lhs.m_lhs, #op##_sr, rhs }; \
} \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->std::enable_if_t< \
Detail::conjunction< \
Detail::negation<Detail::is_##id##_comparable<LhsT, RhsT>>, \
Expand All @@ -274,16 +302,16 @@ namespace Catch {

#define CATCH_INTERNAL_DEFINE_EXPRESSION_OPERATOR( op ) \
template <typename RhsT> \
friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT&& rhs ) \
->std::enable_if_t< \
!std::is_arithmetic<std::remove_reference_t<RhsT>>::value, \
!capture_by_value<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, \
constexpr friend auto operator op( ExprLhs&& lhs, RhsT rhs ) \
->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 All @@ -309,19 +337,23 @@ namespace Catch {
"wrap the expression inside parentheses, or decompose it");
}

auto makeUnaryExpr() const -> UnaryExpr<LhsT> {
constexpr auto makeUnaryExpr() const -> UnaryExpr<LhsT> {
return UnaryExpr<LhsT>{ m_lhs };
}
};

struct Decomposer {
template<typename T, std::enable_if_t<!std::is_arithmetic<std::remove_reference_t<T>>::value, int> = 0>
friend auto operator <= ( Decomposer &&, T && lhs ) -> ExprLhs<T const&> {
template <typename T,
std::enable_if_t<
!capture_by_value<std::remove_reference_t<T>>::value,
int> = 0>
constexpr friend auto operator <= ( Decomposer &&, T && lhs ) -> ExprLhs<T const&> {
return ExprLhs<const T&>{ lhs };
}

template<typename T, std::enable_if_t<std::is_arithmetic<T>::value, int> = 0>
friend auto operator <= ( Decomposer &&, T value ) -> ExprLhs<T> {
template <typename T,
std::enable_if_t<capture_by_value<T>::value, int> = 0>
constexpr friend auto operator <= ( Decomposer &&, T value ) -> ExprLhs<T> {
return ExprLhs<T>{ value };
}
};
Expand Down
71 changes: 69 additions & 2 deletions tests/SelfTest/UsageTests/Compilation.tests.cpp
View file
Expand Up @@ -313,11 +313,12 @@ TEST_CASE("ADL universal operators don't hijack expression deconstruction", "[co
REQUIRE(0 ^ adl::always_true{});
}
TEST_CASE( "#2555 - types that can only be compared with 0 literal (not int/long) are supported", "[compilation][approvals]" ) {
TEST_CASE( "#2555 - types that can only be compared with 0 literal implemented as pointer conversion are supported",
"[compilation][approvals]" ) {
REQUIRE( TypeWithLit0Comparisons{} < 0 );
REQUIRE_FALSE( 0 < TypeWithLit0Comparisons{} );
REQUIRE( TypeWithLit0Comparisons{} <= 0 );
REQUIRE_FALSE( 0 > TypeWithLit0Comparisons{} );
REQUIRE_FALSE( 0 <= TypeWithLit0Comparisons{} );
REQUIRE( TypeWithLit0Comparisons{} > 0 );
REQUIRE_FALSE( 0 > TypeWithLit0Comparisons{} );
Expand All @@ -330,6 +331,72 @@ TEST_CASE( "#2555 - types that can only be compared with 0 literal (not int/long
REQUIRE_FALSE( 0 != TypeWithLit0Comparisons{} );
}
// These tests require `consteval` to propagate through `constexpr` calls
// which is a late DR against C++20.
#if defined( CATCH_CPP20_OR_GREATER ) && defined( __cpp_consteval ) && \
__cpp_consteval >= 202211L
// Can't have internal linkage to avoid warnings
void ZeroLiteralErrorFunc();
namespace {
struct ZeroLiteralConsteval {
template <class T, std::enable_if_t<std::is_same_v<T, int>, int> = 0>
consteval ZeroLiteralConsteval( T zero ) noexcept {
if ( zero != 0 ) { ZeroLiteralErrorFunc(); }
}
};

// Should only be constructible from literal 0. Uses the propagating
// consteval constructor trick (currently used by MSVC, might be used
// by libc++ in the future as well).
struct TypeWithConstevalLit0Comparison {
# define DEFINE_COMP_OP( op ) \
constexpr friend bool operator op( TypeWithConstevalLit0Comparison, \
ZeroLiteralConsteval ) { \
return true; \
} \
constexpr friend bool operator op( ZeroLiteralConsteval, \
TypeWithConstevalLit0Comparison ) { \
return false; \
}

DEFINE_COMP_OP( < )
DEFINE_COMP_OP( <= )
DEFINE_COMP_OP( > )
DEFINE_COMP_OP( >= )
DEFINE_COMP_OP( == )
DEFINE_COMP_OP( != )

#undef DEFINE_COMP_OP
};

} // namespace

namespace Catch {
template <>
struct capture_by_value<TypeWithConstevalLit0Comparison> : std::true_type {};
}

TEST_CASE( "#2555 - types that can only be compared with 0 literal implemented as consteval check are supported",
"[compilation][approvals]" ) {
REQUIRE( TypeWithConstevalLit0Comparison{} < 0 );
REQUIRE_FALSE( 0 < TypeWithConstevalLit0Comparison{} );
REQUIRE( TypeWithConstevalLit0Comparison{} <= 0 );
REQUIRE_FALSE( 0 <= TypeWithConstevalLit0Comparison{} );

REQUIRE( TypeWithConstevalLit0Comparison{} > 0 );
REQUIRE_FALSE( 0 > TypeWithConstevalLit0Comparison{} );
REQUIRE( TypeWithConstevalLit0Comparison{} >= 0 );
REQUIRE_FALSE( 0 >= TypeWithConstevalLit0Comparison{} );

REQUIRE( TypeWithConstevalLit0Comparison{} == 0 );
REQUIRE_FALSE( 0 == TypeWithConstevalLit0Comparison{} );
REQUIRE( TypeWithConstevalLit0Comparison{} != 0 );
REQUIRE_FALSE( 0 != TypeWithConstevalLit0Comparison{} );
}

#endif // C++20 consteval


namespace {
struct MultipleImplicitConstructors {
MultipleImplicitConstructors( double ) {}
Expand Down
23 changes: 14 additions & 9 deletions tests/SelfTest/helpers/type_with_lit_0_comparisons.hpp
View file
Expand Up @@ -12,23 +12,28 @@
#include <type_traits>

// Should only be constructible from literal 0.
// Based on the constructor from pointer trick, used by libstdc++ and libc++
// (formerly also MSVC, but they've moved to consteval int constructor).
// Used by `TypeWithLit0Comparisons` for testing comparison
// ops that only work with literal zero, the way std::*orderings do
struct ZeroLiteralDetector {
constexpr ZeroLiteralDetector( ZeroLiteralDetector* ) noexcept {}
struct ZeroLiteralAsPointer {
constexpr ZeroLiteralAsPointer( ZeroLiteralAsPointer* ) noexcept {}

template <typename T,
typename = std::enable_if_t<!std::is_same<T, int>::value>>
constexpr ZeroLiteralDetector( T ) = delete;
constexpr ZeroLiteralAsPointer( T ) = delete;
};


struct TypeWithLit0Comparisons {
#define DEFINE_COMP_OP( op ) \
friend bool operator op( TypeWithLit0Comparisons, ZeroLiteralDetector ) { \
return true; \
} \
friend bool operator op( ZeroLiteralDetector, TypeWithLit0Comparisons ) { \
return false; \
#define DEFINE_COMP_OP( op ) \
constexpr friend bool operator op( TypeWithLit0Comparisons, \
ZeroLiteralAsPointer ) { \
return true; \
} \
constexpr friend bool operator op( ZeroLiteralAsPointer, \
TypeWithLit0Comparisons ) { \
return false; \
}

DEFINE_COMP_OP( < )
Expand Down

0 comments on commit dc51386

Please sign in to comment.