Correctly compare JSON values.

Builds/VisualStudio2013/RippleD.vcxproj

@@ -2282,7 +2282,7 @@
     </ClInclude>
     <ClInclude Include="..\..\src\ripple\json\Output.h">
     </ClInclude>
-    <ClCompile Include="..\..\src\ripple\json\tests\JsonCpp.test.cpp">
+    <ClCompile Include="..\..\src\ripple\json\tests\json_value.test.cpp">
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='debug|x64'">True</ExcludedFromBuild>
       <ExcludedFromBuild Condition="'$(Configuration)|$(Platform)'=='release|x64'">True</ExcludedFromBuild>
     </ClCompile>

Builds/VisualStudio2013/RippleD.vcxproj.filters

@@ -2976,7 +2976,7 @@
     <ClInclude Include="..\..\src\ripple\json\Output.h">
       <Filter>ripple\json</Filter>
     </ClInclude>
-    <ClCompile Include="..\..\src\ripple\json\tests\JsonCpp.test.cpp">
+    <ClCompile Include="..\..\src\ripple\json\tests\json_value.test.cpp">
       <Filter>ripple\json\tests</Filter>
     </ClCompile>
     <ClCompile Include="..\..\src\ripple\json\tests\Object.test.cpp">

src/ripple/json/impl/json_value.cpp

@@ -443,77 +443,61 @@ Value::type () const
     return type_;
 }
 
-
-int
-Value::compare ( const Value& other )
+static
+int integerCmp (Int i, UInt ui)
 {
-    /*
-    int typeDelta = other.type_ - type_;
-    switch ( type_ )
-    {
-    case nullValue:
+    // All negative numbers are less than all unsigned numbers.
+    if (i < 0)
+        return -1;
 
-       return other.type_ == type_;
-    case intValue:
-       if ( other.type_.isNumeric()
-    case uintValue:
-    case realValue:
-    case booleanValue:
-       break;
-    case stringValue,
-       break;
-    case arrayValue:
-       delete value_.array_;
-       break;
-    case objectValue:
-       delete value_.map_;
-    default:
-       JSON_ASSERT_UNREACHABLE;
-    }
-    */
-    return 0;  // unreachable
+    // All unsigned numbers with bit 0 set are too big for signed integers.
+    if (ui & 0x8000)
+        return 1;
+
+    // Now we can safely compare.
+    return (i < ui) ? -1 : (i == ui) ? 0 : 1;
 }
 
-bool
-Value::operator < ( const Value& other ) const
+bool operator < (const Value& x, const Value& y)
 {
-    int typeDelta = type_ - other.type_;
+    if (auto signum = x.type_ - y.type_)
+    {
+        if (x.type_ == intValue && y.type_ == uintValue)
+            signum = integerCmp (x.value_.int_, y.value_.uint_);
+        else if (x.type_ == uintValue && y.type_ == intValue)
+            signum = - integerCmp (y.value_.int_, x.value_.uint_);
+        return signum < 0;
+    }
 
-    if ( typeDelta )
-        return typeDelta < 0 ? true : false;
-
-    switch ( type_ )
+    switch (x.type_)
     {
     case nullValue:
         return false;
 
     case intValue:
-        return value_.int_ < other.value_.int_;
+        return x.value_.int_ < y.value_.int_;
 
     case uintValue:
-        return value_.uint_ < other.value_.uint_;
+        return x.value_.uint_ < y.value_.uint_;
 
     case realValue:
-        return value_.real_ < other.value_.real_;
+        return x.value_.real_ < y.value_.real_;
 
     case booleanValue:
-        return value_.bool_ < other.value_.bool_;
+        return x.value_.bool_ < y.value_.bool_;
 
     case stringValue:
-        return ( value_.string_ == 0  &&  other.value_.string_ )
-               || ( other.value_.string_
-                    &&  value_.string_
-                    && strcmp ( value_.string_, other.value_.string_ ) < 0 );
+        return (x.value_.string_ == 0  &&  y.value_.string_)
+               || (y.value_.string_ && x.value_.string_ &&
+                   strcmp (x.value_.string_, y.value_.string_) < 0);
 
     case arrayValue:
     case objectValue:
     {
-        int delta = int ( value_.map_->size () - other.value_.map_->size () );
+        if (int signum = int (x.value_.map_->size ()) - y.value_.map_->size ())
+            return signum < 0;
 
-        if ( delta )
-            return delta < 0;
-
-        return (*value_.map_) < (*other.value_.map_);
+        return *x.value_.map_ < *y.value_.map_;
     }
 
     default:
@@ -523,63 +507,43 @@ Value::operator < ( const Value& other ) const
     return 0;  // unreachable
 }
 
-bool
-Value::operator <= ( const Value& other ) const
+bool operator== (const Value& x, const Value& y)
 {
-    return ! (other > *this);
-}
-
-bool
-Value::operator >= ( const Value& other ) const
+    if (x.type_ != y.type_)
     {
-    return ! (*this < other);
-}
-
-bool
-Value::operator > ( const Value& other ) const
-{
-    return other < *this;
-}
-
-bool
-Value::operator == ( const Value& other ) const
-{
-    //if ( type_ != other.type_ )
-    // GCC 2.95.3 says:
-    // attempt to take address of bit-field structure member `Json::Value::type_'
-    // Beats me, but a temp solves the problem.
-    int temp = other.type_;
-
-    if ( type_ != temp )
+        if (x.type_ == intValue && y.type_ == uintValue)
+            return ! integerCmp (x.value_.int_, y.value_.uint_);
+        if (x.type_ == uintValue && y.type_ == intValue)
+            return ! integerCmp (y.value_.int_, x.value_.uint_);
         return false;
+    }
 
-    switch ( type_ )
+    switch (x.type_)
     {
     case nullValue:
         return true;
 
     case intValue:
-        return value_.int_ == other.value_.int_;
+        return x.value_.int_ == y.value_.int_;
 
     case uintValue:
-        return value_.uint_ == other.value_.uint_;
+        return x.value_.uint_ == y.value_.uint_;
 
     case realValue:
-        return value_.real_ == other.value_.real_;
+        return x.value_.real_ == y.value_.real_;
 
     case booleanValue:
-        return value_.bool_ == other.value_.bool_;
+        return x.value_.bool_ == y.value_.bool_;
 
     case stringValue:
-        return ( value_.string_ == other.value_.string_ )
-               || ( other.value_.string_
-                    &&  value_.string_
-                    && strcmp ( value_.string_, other.value_.string_ ) == 0 );
+        return x.value_.string_ == y.value_.string_
+               || (y.value_.string_ && x.value_.string_ &&
+                    ! strcmp (x.value_.string_, y.value_.string_));
 
     case arrayValue:
     case objectValue:
-        return value_.map_->size () == other.value_.map_->size ()
-               && (*value_.map_) == (*other.value_.map_);
+        return x.value_.map_->size () == y.value_.map_->size ()
+               && *x.value_.map_ == *y.value_.map_;
 
     default:
         JSON_ASSERT_UNREACHABLE;
@@ -588,12 +552,6 @@ Value::operator == ( const Value& other ) const
     return 0;  // unreachable
 }
 
-bool
-Value::operator != ( const Value& other ) const
-{
-    return ! ( *this == other );
-}
-
 const char*
 Value::asCString () const
 {

src/ripple/json/json_value.h

@@ -89,6 +89,18 @@ private:
     const char* str_;
 };
 
+inline bool operator!= (StaticString x, StaticString y)
+{
+    // TODO(tom): could I use x != y here because StaticStrings are supposed to
+    // be unique?
+    return strcmp (x, y);
+}
+
+inline bool operator== (StaticString x, StaticString y)
+{
+    return ! (x != y);
+}
+
 inline bool operator== (std::string const& x, StaticString y)
 {
     return x == y.c_str();
@@ -234,16 +246,6 @@ public:
 
     ValueType type () const;
 
-    bool operator < ( const Value& other ) const;
-    bool operator <= ( const Value& other ) const;
-    bool operator >= ( const Value& other ) const;
-    bool operator > ( const Value& other ) const;
-
-    bool operator == ( const Value& other ) const;
-    bool operator != ( const Value& other ) const;
-
-    int compare ( const Value& other );
-
     const char* asCString () const;
     std::string asString () const;
     Int asInt () const;
@@ -369,6 +371,9 @@ public:
     iterator begin ();
     iterator end ();
 
+    friend bool operator== (const Value&, const Value&);
+    friend bool operator< (const Value&, const Value&);
+
 private:
     Value& resolveReference ( const char* key,
                               bool isStatic );
@@ -387,6 +392,34 @@ private:
     int allocated_ : 1;     // Notes: if declared as bool, bitfield is useless.
 };
 
+bool operator== (const Value&, const Value&);
+
+inline
+bool operator!= (const Value& x, const Value& y)
+{
+    return ! (x == y);
+}
+
+bool operator< (const Value&, const Value&);
+
+inline
+bool operator<= (const Value& x, const Value& y)
+{
+    return ! (y < x);
+}
+
+inline
+bool operator> (const Value& x, const Value& y)
+{
+    return y < x;
+}
+
+inline
+bool operator>= (const Value& x, const Value& y)
+{
+    return ! (x < y);
+}
+
 /** \brief Experimental do not use: Allocator to customize member name and string value memory management done by Value.
  *
  * - makeMemberName() and releaseMemberName() are called to respectively duplicate and

src/ripple/json/tests/json_value.test.cpp

@@ -25,7 +25,7 @@
 
 namespace ripple {
 
-class JsonCpp_test : public beast::unit_test::suite
+class json_value_test : public beast::unit_test::suite
 {
 public:
     void test_bad_json ()
@@ -139,15 +139,72 @@ public:
         pass ();
     }
 
+    void
+    test_comparisons()
+    {
+        Json::Value a, b;
+        auto testEquals = [&] (std::string const& name) {
+            expect (a == b, "a == b " + name);
+            expect (a <= b, "a <= b " + name);
+            expect (a >= b, "a >= b " + name);
+
+            expect (! (a != b), "! (a != b) " + name);
+            expect (! (a < b), "! (a < b) " + name);
+            expect (! (a > b), "! (a > b) " + name);
+
+            expect (b == a, "b == a " + name);
+            expect (b <= a, "b <= a " + name);
+            expect (b >= a, "b >= a " + name);
+
+            expect (! (b != a), "! (b != a) " + name);
+            expect (! (b < a), "! (b < a) " + name);
+            expect (! (b > a), "! (b > a) " + name);
+        };
+
+        auto testGreaterThan = [&] (std::string const& name) {
+            expect (! (a == b), "! (a == b) " + name);
+            expect (! (a <= b), "! (a <= b) " + name);
+            expect (a >= b, "a >= b " + name);
+
+            expect (a != b, "a != b " + name);
+            expect (! (a < b), "! (a < b) " + name);
+            expect (a > b, "a > b " + name);
+
+            expect (! (b == a), "! (b == a) " + name);
+            expect (b <= a, "b <= a " + name);
+            expect (! (b >= a), "! (b >= a) " + name);
+
+            expect (b != a, "b != a " + name);
+            expect (b < a, "b < a " + name);
+            expect (! (b > a), "! (b > a) " + name);
+        };
+
+        a["a"] = Json::UInt (0);
+        b["a"] = Json::Int (0);
+        testEquals ("zero");
+
+        b["a"] = Json::Int (-1);
+        testGreaterThan ("negative");
+
+        Json::Int big = std::numeric_limits<int>::max();
+        Json::UInt bigger = big;
+        bigger++;
+
+        a["a"] = bigger;
+        b["a"] = big;
+        testGreaterThan ("big");
+    }
+
     void run ()
     {
         test_bad_json ();
         test_edge_cases ();
         test_copy ();
         test_move ();
+        test_comparisons ();
     }
 };
 
-BEAST_DEFINE_TESTSUITE(JsonCpp,json,ripple);
+BEAST_DEFINE_TESTSUITE(json_value, json, ripple);
 
 } // ripple

src/ripple/unity/json.cpp

@@ -42,7 +42,7 @@
 #include <ripple/json/impl/Object.cpp>
 #include <ripple/json/impl/Output.cpp>
 
-#include <ripple/json/tests/JsonCpp.test.cpp>
+#include <ripple/json/tests/json_value.test.cpp>
 #include <ripple/json/tests/Object.test.cpp>
 #include <ripple/json/tests/Output.test.cpp>
 #include <ripple/json/tests/Writer.test.cpp>