From f6758c2967bc5c7fcd3333dc08871f0793234355 Mon Sep 17 00:00:00 2001 From: Phil Nash Date: Tue, 15 Mar 2011 22:22:19 +0000 Subject: [PATCH] Added special case logic for signed/ unsigned comparisons involving negative numbers and added test cases to cover them --- Test/ConditionTests.cpp | 33 ++++++++++++++ internal/catch_capture.hpp | 18 ++++---- internal/catch_evaluate.hpp | 87 +++++++++++++++++++++++++++++++------ 3 files changed, 115 insertions(+), 23 deletions(-) diff --git a/Test/ConditionTests.cpp b/Test/ConditionTests.cpp index b1010169..727fb9eb 100644 --- a/Test/ConditionTests.cpp +++ b/Test/ConditionTests.cpp @@ -180,6 +180,39 @@ TEST_CASE( "./succeeding/conditions/int literals", "Comparisons with int literal REQUIRE( 62270208023445 > ul ); } +// Because we have to do some conversions when comparing certain signed/ unsigned types (to avoid +// spurious warnings when comparing integer literals with unsigned integers), we have a set of tests +// here to confirm that the behaviour is correct at the boundaries +TEST_CASE( "./succeeding/conditions/unsigned-negative", "Comparisons between negative signed and unsigned ints, expected to succeed" ) +{ + using namespace Catch::Generators; + + int negative = GENERATE( values( -1, -2, (std::numeric_limits::min)() ) ); + unsigned int ui = GENERATE( values( 0u, 1u, 2u, (std::numeric_limits::max)() ) ); + + CHECK( ui > negative ); + CHECK( negative < ui ); + CHECK( ui >= negative ); + CHECK( negative <= ui ); + CHECK( ui != negative ); + CHECK( negative != ui ); +} + +TEST_CASE( "./failing/conditions/unsigned-negative", "Comparisons between negative signed and unsigned ints, expected to fail" ) +{ + using namespace Catch::Generators; + + int negative = GENERATE( values( -1, -2, (std::numeric_limits::min)() ) ); + unsigned int ui = GENERATE( values( 0u, 1u, 2u, (std::numeric_limits::max)() ) ); + + CHECK( ui < negative ); + CHECK( negative > ui ); + CHECK( ui <= negative ); + CHECK( negative >= ui ); + CHECK( ui == negative ); + CHECK( negative == ui ); +} + // Not (!) tests // The problem with the ! operator is that it has right-to-left associativity. // This means we can't isolate it when we decompose. The simple REQUIRE( !false ) form, therefore, diff --git a/internal/catch_capture.hpp b/internal/catch_capture.hpp index 09286568..0ba08d21 100644 --- a/internal/catch_capture.hpp +++ b/internal/catch_capture.hpp @@ -242,17 +242,17 @@ private: } /////////////////////////////////////////////////////////////////////////// - template + template MutableResultInfo& captureExpression ( const T1& lhs, const T2& rhs ) { - setResultType( compare( lhs, rhs ) ? ResultWas::Ok : ResultWas::ExpressionFailed ); + setResultType( Internal::compare( lhs, rhs ) ? ResultWas::Ok : ResultWas::ExpressionFailed ); m_lhs = toString( lhs ); m_rhs = toString( rhs ); - m_op = OperatorTraits::getName(); + m_op = Internal::OperatorTraits::getName(); return *this; } @@ -282,7 +282,7 @@ public: const RhsT& rhs ) { - return m_result.captureExpression( m_lhs, rhs ); + return m_result.captureExpression( m_lhs, rhs ); } /////////////////////////////////////////////////////////////////////////// @@ -292,7 +292,7 @@ public: const RhsT& rhs ) { - return m_result.captureExpression( m_lhs, rhs ); + return m_result.captureExpression( m_lhs, rhs ); } /////////////////////////////////////////////////////////////////////////// @@ -302,7 +302,7 @@ public: const RhsT& rhs ) { - return m_result.captureExpression( m_lhs, rhs ); + return m_result.captureExpression( m_lhs, rhs ); } /////////////////////////////////////////////////////////////////////////// @@ -312,7 +312,7 @@ public: const RhsT& rhs ) { - return m_result.captureExpression( m_lhs, rhs ); + return m_result.captureExpression( m_lhs, rhs ); } /////////////////////////////////////////////////////////////////////////// @@ -322,7 +322,7 @@ public: const RhsT& rhs ) { - return m_result.captureExpression( m_lhs, rhs ); + return m_result.captureExpression( m_lhs, rhs ); } /////////////////////////////////////////////////////////////////////////// @@ -332,7 +332,7 @@ public: const RhsT& rhs ) { - return m_result.captureExpression( m_lhs, rhs ); + return m_result.captureExpression( m_lhs, rhs ); } /////////////////////////////////////////////////////////////////////////// diff --git a/internal/catch_evaluate.hpp b/internal/catch_evaluate.hpp index 46c6f8a0..2d020ca5 100644 --- a/internal/catch_evaluate.hpp +++ b/internal/catch_evaluate.hpp @@ -14,6 +14,8 @@ #define TWOBLUECUBES_CATCH_EVALUATE_HPP_INCLUDED namespace Catch +{ +namespace Internal { enum Operator { @@ -46,13 +48,32 @@ namespace Catch template<> struct OperatorTraits{ static const char* getName(){ return ">="; } }; + // Because we capture the LHS and RHS of a binary condition expression by reference, then + // compare the referenced values later, we may get compiler warnings when comparing unsigned + // integer types with integer literals (which are signed - int or long, specifically). + // To avoid this warning we filter out the problem cases as a set of overloads of the compare + // function. In those overloads we cast the unsigned type to its signed equivalent then + // perform the comparison. However we also have to handle the case where the signed value is + // negative. Comparing a negative value with an unsigned value (which will always be positive) + // has fixed logic per operator, so this is captured seperately as an enum value. + enum LostSign + { + None = 0, + LhsSignWasLost = 1, + RhsSignWasLost = 2 + }; + + // So the compare overloads can be operator agnostic we convey the operator as a template + // enum, which is used to specialise an Evaluator for doing the comparison. template class Evaluator{}; template struct Evaluator { - static bool evaluate( const T1& lhs, const T2& rhs ) + enum{ failsWhen = LhsSignWasLost | RhsSignWasLost }; + + static bool evaluate( const T1& lhs, const T2& rhs) { return lhs == rhs; } @@ -60,6 +81,8 @@ namespace Catch template struct Evaluator { + enum{ failsWhen = None }; + static bool evaluate( const T1& lhs, const T2& rhs ) { return lhs != rhs; @@ -68,6 +91,8 @@ namespace Catch template struct Evaluator { + enum{ failsWhen = RhsSignWasLost }; + static bool evaluate( const T1& lhs, const T2& rhs ) { return lhs < rhs; @@ -76,6 +101,8 @@ namespace Catch template struct Evaluator { + enum{ failsWhen = LhsSignWasLost }; + static bool evaluate( const T1& lhs, const T2& rhs ) { return lhs > rhs; @@ -84,6 +111,8 @@ namespace Catch template struct Evaluator { + enum{ failsWhen = LhsSignWasLost }; + static bool evaluate( const T1& lhs, const T2& rhs ) { return lhs >= rhs; @@ -92,12 +121,40 @@ namespace Catch template struct Evaluator { + enum{ failsWhen = RhsSignWasLost }; + static bool evaluate( const T1& lhs, const T2& rhs ) { return lhs <= rhs; } }; + // All the special case signed/ unsigned overloads of compare forward to this function, + // which, for negative numbers checks the special case fixed logic, otherwise forwards on + // to the specialised Evaluator for the operator enum + template + bool applyEvaluator( const T1& lhs, const T2& rhs, LostSign lostSign ) + { + typedef Evaluator EvaluatorType; + return lostSign == None + ? EvaluatorType::evaluate( lhs, rhs ) + : ( EvaluatorType::failsWhen & lostSign ) != lostSign; + } + + + template + LostSign testLhsSign( T lhs ) + { + return lhs < 0 ? LhsSignWasLost : None; + } + + template + LostSign testRhsSign( T rhs ) + { + return rhs < 0 ? RhsSignWasLost : None; + } + + // "base" overload template bool compare( const T1& lhs, const T2& rhs ) { @@ -107,58 +164,60 @@ namespace Catch // unsigned X to int template bool compare( unsigned int lhs, int rhs ) { - return Evaluator::evaluate( lhs, static_cast( rhs ) ); + return applyEvaluator( lhs, static_cast( rhs ), testRhsSign( rhs ) ); } template bool compare( unsigned long lhs, int rhs ) { - return Evaluator::evaluate( lhs, static_cast( rhs ) ); + return applyEvaluator( lhs, static_cast( rhs ), testRhsSign( rhs ) ); } template bool compare( unsigned char lhs, int rhs ) { - return Evaluator::evaluate( lhs, static_cast( rhs ) ); + return applyEvaluator( lhs, static_cast( rhs ), testRhsSign( rhs ) ); } // unsigned X to long template bool compare( unsigned int lhs, long rhs ) { - return Evaluator::evaluate( lhs, static_cast( rhs ) ); + return applyEvaluator( lhs, static_cast( rhs ), testRhsSign( rhs ) ); } template bool compare( unsigned long lhs, long rhs ) { - return Evaluator::evaluate( lhs, static_cast( rhs ) ); + return applyEvaluator( lhs, static_cast( rhs ), testRhsSign( rhs ) ); } template bool compare( unsigned char lhs, long rhs ) { - return Evaluator::evaluate( lhs, static_cast( rhs ) ); + return applyEvaluator( lhs, static_cast( rhs ), testRhsSign( rhs ) ); } // int to unsigned X template bool compare( int lhs, unsigned int rhs ) { - return Evaluator::evaluate( static_cast( lhs ), rhs ); + return applyEvaluator( static_cast( lhs ), rhs, testLhsSign( lhs ) ); } template bool compare( int lhs, unsigned long rhs ) { - return Evaluator::evaluate( static_cast( lhs ), rhs ); + return applyEvaluator( static_cast( lhs ), rhs, testLhsSign( lhs ) ); } template bool compare( int lhs, unsigned char rhs ) { - return Evaluator::evaluate( static_cast( lhs ), rhs ); + return applyEvaluator( static_cast( lhs ), rhs, testLhsSign( lhs ) ); } // long to unsigned X template bool compare( long lhs, unsigned int rhs ) { - return Evaluator::evaluate( static_cast( lhs ), rhs ); + return applyEvaluator( static_cast( lhs ), rhs, testLhsSign( lhs ) ); } template bool compare( long lhs, unsigned long rhs ) { - return Evaluator::evaluate( static_cast( lhs ), rhs ); + return applyEvaluator( static_cast( lhs ), rhs, testLhsSign( lhs ) ); } template bool compare( long lhs, unsigned char rhs ) { - return Evaluator::evaluate( static_cast( lhs ), rhs ); + return applyEvaluator( static_cast( lhs ), rhs, testLhsSign( lhs ) ); } -} + +} // end of namespace Internal +} // end of namespace Catch #endif // TWOBLUECUBES_CATCH_EVALUATE_HPP_INCLUDED