Added special case logic for signed/ unsigned comparisons involving negative numbers and added test cases to cover them

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<int>::min)() ) );
+    unsigned int ui = GENERATE( values( 0u, 1u, 2u, (std::numeric_limits<unsigned int>::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<int>::min)() ) );
+    unsigned int ui = GENERATE( values( 0u, 1u, 2u, (std::numeric_limits<unsigned int>::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,

internal/catch_capture.hpp

@@ -242,17 +242,17 @@ private:
     }    
 
     ///////////////////////////////////////////////////////////////////////////
-    template<Operator Op, typename T1, typename T2>    
+    template<Internal::Operator Op, typename T1, typename T2>    
     MutableResultInfo& captureExpression
     (
         const T1& lhs, 
         const T2& rhs
     )
     {
-        setResultType( compare<Op>( lhs, rhs ) ? ResultWas::Ok : ResultWas::ExpressionFailed );
+        setResultType( Internal::compare<Op>( lhs, rhs ) ? ResultWas::Ok : ResultWas::ExpressionFailed );
         m_lhs = toString( lhs );
         m_rhs = toString( rhs );
-        m_op = OperatorTraits<Op>::getName();
+        m_op = Internal::OperatorTraits<Op>::getName();
         return *this;
     }
     
@@ -282,7 +282,7 @@ public:
         const RhsT& rhs
     )
     {
-        return m_result.captureExpression<IsEqualTo>( m_lhs, rhs );
+        return m_result.captureExpression<Internal::IsEqualTo>( m_lhs, rhs );
     }
     
     ///////////////////////////////////////////////////////////////////////////
@@ -292,7 +292,7 @@ public:
         const RhsT& rhs
     )
     {
-        return m_result.captureExpression<IsNotEqualTo>( m_lhs, rhs );
+        return m_result.captureExpression<Internal::IsNotEqualTo>( m_lhs, rhs );
     }
     
     ///////////////////////////////////////////////////////////////////////////
@@ -302,7 +302,7 @@ public:
         const RhsT& rhs
     )
     {
-        return m_result.captureExpression<IsLessThan>( m_lhs, rhs );
+        return m_result.captureExpression<Internal::IsLessThan>( m_lhs, rhs );
     }
     
     ///////////////////////////////////////////////////////////////////////////
@@ -312,7 +312,7 @@ public:
         const RhsT& rhs
     )
     {
-        return m_result.captureExpression<IsGreaterThan>( m_lhs, rhs );
+        return m_result.captureExpression<Internal::IsGreaterThan>( m_lhs, rhs );
     }
     
     ///////////////////////////////////////////////////////////////////////////
@@ -322,7 +322,7 @@ public:
         const RhsT& rhs
     )
     {
-        return m_result.captureExpression<IsLessThanOrEqualTo>( m_lhs, rhs );
+        return m_result.captureExpression<Internal::IsLessThanOrEqualTo>( m_lhs, rhs );
     }
     
     ///////////////////////////////////////////////////////////////////////////
@@ -332,7 +332,7 @@ public:
         const RhsT& rhs
     )
     {
-        return m_result.captureExpression<IsGreaterThanOrEqualTo>( m_lhs, rhs );
+        return m_result.captureExpression<Internal::IsGreaterThanOrEqualTo>( m_lhs, rhs );
     }
 
     ///////////////////////////////////////////////////////////////////////////

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<IsGreaterThanOrEqualTo>{ 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<typename T1, typename T2, Operator Op>
     class Evaluator{};
     
     template<typename T1, typename T2>
     struct Evaluator<T1, T2, IsEqualTo>
     {
-        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<typename T1, typename T2>
     struct Evaluator<T1, T2, IsNotEqualTo>
     {
+        enum{ failsWhen = None };
+
         static bool evaluate( const T1& lhs, const T2& rhs )
         {
             return lhs != rhs;
@@ -68,6 +91,8 @@ namespace Catch
     template<typename T1, typename T2>
     struct Evaluator<T1, T2, IsLessThan>
     {
+        enum{ failsWhen = RhsSignWasLost };
+        
         static bool evaluate( const T1& lhs, const T2& rhs )
         {
             return lhs < rhs;
@@ -76,6 +101,8 @@ namespace Catch
     template<typename T1, typename T2>
     struct Evaluator<T1, T2, IsGreaterThan>
     {
+        enum{ failsWhen = LhsSignWasLost };
+
         static bool evaluate( const T1& lhs, const T2& rhs )
         {
             return lhs > rhs;
@@ -84,6 +111,8 @@ namespace Catch
     template<typename T1, typename T2>
     struct Evaluator<T1, T2, IsGreaterThanOrEqualTo>
     {
+        enum{ failsWhen = LhsSignWasLost };
+        
         static bool evaluate( const T1& lhs, const T2& rhs )
         {
             return lhs >= rhs;
@@ -92,12 +121,40 @@ namespace Catch
     template<typename T1, typename T2>
     struct Evaluator<T1, T2, IsLessThanOrEqualTo>
     {
+        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<Operator Op, typename T1, typename T2>
+    bool applyEvaluator( const T1& lhs, const T2& rhs, LostSign lostSign )
+    {
+        typedef Evaluator<T1, T2, Op> EvaluatorType;
+        return lostSign == None
+            ? EvaluatorType::evaluate( lhs, rhs )
+            : ( EvaluatorType::failsWhen & lostSign ) != lostSign;
+    }
+    
+    
+    template<typename T>
+    LostSign testLhsSign( T lhs )
+    {
+        return lhs < 0 ? LhsSignWasLost : None;
+    }
+
+    template<typename T>
+    LostSign testRhsSign( T rhs )
+    {
+        return rhs < 0 ? RhsSignWasLost : None;
+    }
+    
+    // "base" overload
     template<Operator Op, typename T1, typename T2>
     bool compare( const T1& lhs, const T2& rhs )
     {
@@ -107,58 +164,60 @@ namespace Catch
     // unsigned X to int
     template<Operator Op> bool compare( unsigned int lhs, int rhs )
     {
-        return Evaluator<unsigned int, unsigned int, Op>::evaluate( lhs, static_cast<unsigned int>( rhs ) );
+        return applyEvaluator<Op>( lhs, static_cast<unsigned int>( rhs ), testRhsSign( rhs ) );
     }
     template<Operator Op> bool compare( unsigned long lhs, int rhs )
     {
-        return Evaluator<unsigned long, unsigned int, Op>::evaluate( lhs, static_cast<unsigned int>( rhs ) );
+        return applyEvaluator<Op>( lhs, static_cast<unsigned int>( rhs ), testRhsSign( rhs )  );
     }
     template<Operator Op> bool compare( unsigned char lhs, int rhs )
     {
-        return Evaluator<unsigned char, unsigned int, Op>::evaluate( lhs, static_cast<unsigned int>( rhs ) );
+        return applyEvaluator<Op>( lhs, static_cast<unsigned int>( rhs ), testRhsSign( rhs )  );
     }
     
     // unsigned X to long
     template<Operator Op> bool compare( unsigned int lhs, long rhs )
     {
-        return Evaluator<unsigned int, unsigned long, Op>::evaluate( lhs, static_cast<unsigned long>( rhs ) );
+        return applyEvaluator<Op>( lhs, static_cast<unsigned long>( rhs ), testRhsSign( rhs )  );
     }
     template<Operator Op> bool compare( unsigned long lhs, long rhs )
     {
-        return Evaluator<unsigned long, unsigned long, Op>::evaluate( lhs, static_cast<unsigned long>( rhs ) );
+        return applyEvaluator<Op>( lhs, static_cast<unsigned long>( rhs ), testRhsSign( rhs )  );
     }
     template<Operator Op> bool compare( unsigned char lhs, long rhs )
     {
-        return Evaluator<unsigned char, unsigned long, Op>::evaluate( lhs, static_cast<unsigned long>( rhs ) );
+        return applyEvaluator<Op>( lhs, static_cast<unsigned long>( rhs ), testRhsSign( rhs )  );
     }
     
     // int to unsigned X
     template<Operator Op> bool compare( int lhs, unsigned int rhs )
     {
-        return Evaluator<unsigned int, unsigned int, Op>::evaluate( static_cast<unsigned int>( lhs ), rhs );
+        return applyEvaluator<Op>( static_cast<unsigned int>( lhs ), rhs, testLhsSign( lhs )  );
     }
     template<Operator Op> bool compare( int lhs, unsigned long rhs )
     {
-        return Evaluator<unsigned int, unsigned long, Op>::evaluate( static_cast<unsigned int>( lhs ), rhs );
+        return applyEvaluator<Op>( static_cast<unsigned int>( lhs ), rhs, testLhsSign( lhs ) );
     }
     template<Operator Op> bool compare( int lhs, unsigned char rhs )
     {
-        return Evaluator<unsigned int, unsigned char, Op>::evaluate( static_cast<unsigned int>( lhs ), rhs );
+        return applyEvaluator<Op>( static_cast<unsigned int>( lhs ), rhs, testLhsSign( lhs ) );
     }
     
     // long to unsigned X
     template<Operator Op> bool compare( long lhs, unsigned int rhs )
     {
-        return Evaluator<unsigned long, unsigned int, Op>::evaluate( static_cast<unsigned long>( lhs ), rhs );
+        return applyEvaluator<Op>( static_cast<unsigned long>( lhs ), rhs, testLhsSign( lhs ) );
     }
     template<Operator Op> bool compare( long lhs, unsigned long rhs )
     {
-        return Evaluator<unsigned long, unsigned long, Op>::evaluate( static_cast<unsigned long>( lhs ), rhs );
+        return applyEvaluator<Op>( static_cast<unsigned long>( lhs ), rhs, testLhsSign( lhs ) );
     }
     template<Operator Op> bool compare( long lhs, unsigned char rhs )
     {
-        return Evaluator<unsigned long, unsigned char, Op>::evaluate( static_cast<unsigned long>( lhs ), rhs );
+        return applyEvaluator<Op>( static_cast<unsigned long>( lhs ), rhs, testLhsSign( lhs ) );
     }
-}
+    
+} // end of namespace Internal
+} // end of namespace Catch
 
 #endif // TWOBLUECUBES_CATCH_EVALUATE_HPP_INCLUDED