[FOLD] Review feedback #4:

* More test cases for XRPAmount's mulratio function. * Improve TaggedFee::json to work with non-integral types. Add tests.
ximinez · Oct 2, 2019 · dc36ccc · dc36ccc
1 parent 1948d80
commit dc36ccc
Show file tree

Hide file tree

Showing 4 changed files with 87 additions and 4 deletions.
diff --git a/src/ripple/basics/XRPAmount.h b/src/ripple/basics/XRPAmount.h
@@ -221,7 +221,9 @@ class XRPAmount
     Json::Value
     json () const
     {
-        static_assert(std::is_signed_v<value_type>, "Expected XRPAmount to be signed");
+        static_assert(std::is_signed_v<value_type> &&
+            std::is_integral_v<value_type>,
+            "Expected XRPAmount to be a signed integral type");
 
         constexpr auto min = std::numeric_limits<Json::Int>::min();
         constexpr auto max = std::numeric_limits<Json::Int>::max();

diff --git a/src/ripple/basics/feeunits.h b/src/ripple/basics/feeunits.h
@@ -276,10 +276,19 @@ class TaggedFee
         return fee_;
     }
 
-    Json::Value
+    template<class T = value_type>
+    std::enable_if_t<is_usable_unit_v<TaggedFee> &&
+        !std::is_integral_v<T>, Json::Value>
+    json () const
+    {
+        return fee_;
+    }
+
+    template<class T = value_type>
+    std::enable_if_t<is_usable_unit_v<TaggedFee> &&
+        std::is_integral_v<T>, Json::Value>
     json () const
     {
-        static_assert(is_usable_unit_v<TaggedFee>, "Invalid unit type");
         using jsontype = std::conditional_t<std::is_signed_v<value_type>,
             Json::Int, Json::UInt>;
 
@@ -293,7 +302,6 @@ class TaggedFee
         return static_cast<jsontype>(fee_);
     }
 
-
     /** Returns the underlying value. Code SHOULD NOT call this
         function unless the type has been abstracted away,
         e.g. in a templated function.

diff --git a/src/test/basics/XRPAmount_test.cpp b/src/test/basics/XRPAmount_test.cpp
@@ -143,6 +143,13 @@ class XRPAmount_test : public beast::unit_test::suite
             BEAST_EXPECT(big == mulRatio (big, maxUInt32, maxUInt32, true));
             // rounding mode shouldn't matter as the result is exact
             BEAST_EXPECT(big == mulRatio (big, maxUInt32, maxUInt32, false));
+
+            // multiply and divide by values that would overflow if done naively,
+            // and check that it gives the correct answer
+            big -= 0xf; // Subtract a little so it's divisable by 4
+            BEAST_EXPECT(mulRatio(big, 3, 4, false) == (big / 4) * 3);
+            BEAST_EXPECT(mulRatio(big, 3, 4, true) == (big / 4) * 3);
+            BEAST_EXPECT((big * 3) / 4 != (big / 4) * 3);
         }
 
         {
@@ -151,6 +158,12 @@ class XRPAmount_test : public beast::unit_test::suite
             BEAST_EXPECT(big == mulRatio (big, maxUInt32, maxUInt32, true));
             // rounding mode shouldn't matter as the result is exact
             BEAST_EXPECT(big == mulRatio (big, maxUInt32, maxUInt32, false));
+
+            // multiply and divide by values that would overflow if done naively,
+            // and check that it gives the correct answer
+            BEAST_EXPECT(mulRatio(big, 3, 4, false) == (big / 4) * 3);
+            BEAST_EXPECT(mulRatio(big, 3, 4, true) == (big / 4) * 3);
+            BEAST_EXPECT((big * 3) / 4 != (big / 4) * 3);
         }
 
         {

diff --git a/src/test/basics/feeunits_test.cpp b/src/test/basics/feeunits_test.cpp
@@ -101,6 +101,66 @@ class feeunits_test
                 feeunit::drop_tag>));
             BEAST_EXPECT((std::is_same_v<decltype(drops), XRPAmount>));
         }
+
+        // Json value functionality
+        {
+            FeeUnit32 x{std::numeric_limits<std::uint32_t>::max()};
+            auto y = x.json();
+            BEAST_EXPECT(y.type() == Json::uintValue);
+            BEAST_EXPECT(y == Json::Value{x.fee()});
+        }
+
+        {
+            FeeUnit32 x{std::numeric_limits<std::uint32_t>::min()};
+            auto y = x.json();
+            BEAST_EXPECT(y.type() == Json::uintValue);
+            BEAST_EXPECT(y == Json::Value{x.fee()});
+        }
+
+        {
+            FeeUnit64 x{std::numeric_limits<std::uint64_t>::max()};
+            auto y = x.json();
+            BEAST_EXPECT(y.type() == Json::uintValue);
+            BEAST_EXPECT(y ==
+                Json::Value{std::numeric_limits<std::uint32_t>::max()});
+        }
+
+        {
+            FeeUnit64 x{std::numeric_limits<std::uint64_t>::min()};
+            auto y = x.json();
+            BEAST_EXPECT(y.type() == Json::uintValue);
+            BEAST_EXPECT(y == Json::Value{x.fee()});
+        }
+
+        {
+            FeeLevelDouble x{std::numeric_limits<double>::max()};
+            auto y = x.json();
+            BEAST_EXPECT(y.type() == Json::realValue);
+            BEAST_EXPECT(y == Json::Value{std::numeric_limits<double>::max()});
+        }
+
+        {
+            FeeLevelDouble x{std::numeric_limits<double>::min()};
+            auto y = x.json();
+            BEAST_EXPECT(y.type() == Json::realValue);
+            BEAST_EXPECT(y == Json::Value{std::numeric_limits<double>::min()});
+        }
+
+        {
+            XRPAmount x{std::numeric_limits<std::int64_t>::max()};
+            auto y = x.json();
+            BEAST_EXPECT(y.type() == Json::intValue);
+            BEAST_EXPECT(y ==
+                Json::Value{std::numeric_limits<std::int32_t>::max()});
+        }
+
+        {
+            XRPAmount x{std::numeric_limits<std::int64_t>::min()};
+            auto y = x.json();
+            BEAST_EXPECT(y.type() == Json::intValue);
+            BEAST_EXPECT(y ==
+                Json::Value{std::numeric_limits<std::int32_t>::min()});
+        }
     }
 };