[FOLD] Review feedback from @mellery451:

* Convert scaleFeeLoad worker functions to lambdas. * Simplify to_string(quantity). * Rename detail::clamp to detail::clamp32.
XRPLF · Jun 26, 2019 · b8f7fa0 · b8f7fa0
1 parent 1ba0d03
commit b8f7fa0
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 100 deletions.
diff --git a/src/ripple/app/misc/impl/LoadFeeTrack.cpp b/src/ripple/app/misc/impl/LoadFeeTrack.cpp
@@ -82,22 +82,7 @@ LoadFeeTrack::lowerLocalFee ()
 
 //------------------------------------------------------------------------------
 
-// NIKB TODO: Once we get C++17, we can replace lowestTerms
-//            with this:
-//
-// template <class T1, class T2,
-//     class = std::enable_if_t<
-//         std::is_integral_v<T1> &&
-//         std::is_integral_v<T2>>
-// >
-// void lowestTerms(T1& a,  T2& b)
-// {
-//     if (auto const gcd = std::gcd(a, b))
-//     {
-//         a /= gcd;
-//         b /= gcd;
-//     }
-// }
+// EHENNIS TODO: Once we get C++17, we can replace gcd with std::gcd
 
 template <class T1, class T2,
     class = std::enable_if_t <
@@ -122,85 +107,44 @@ std::uint64_t gcd(T1 const a, T2 const b)
     return x;
 }
 
-template <class T1, class T2,
-    class = std::enable_if_t <
-        std::is_unsigned<T1>::value &&
-        sizeof(T1) <= sizeof(std::uint64_t) >,
-    class = std::enable_if_t <
-        std::is_unsigned<T2>::value &&
-        sizeof(T2) <= sizeof(std::uint64_t) >
->
-void lowestTerms(T1& a,  T2& b)
-{
-    if (auto const g = gcd(a, b))
-    {
-        a /= g;
-        b /= g;
-    }
-}
-
-// Normally, these types wouldn't be mathematically compatible.
-// For this file only, it's ok.
-template <class T1, class T2, class Unit1,
-    class = std::enable_if_t <
-        std::is_unsigned<T1>::value &&
-        sizeof(T1) <= sizeof(std::uint64_t) >,
-    class = std::enable_if_t <
-        std::is_unsigned<T2>::value &&
-        sizeof(T2) <= sizeof(std::uint64_t)>,
-    class = std::enable_if_t <
-        std::is_same_v<Unit1, units::feeunit_unit> ||
-        std::is_same_v<Unit1, units::drop_unit>
-    >
->
-void lowestTerms(boost::units::quantity<Unit1, T1>& a,
-    FeeUnit<T2>& b)
-{
-    if (auto const g = gcd(a.value(), b.value()))
-    {
-        a /= g;
-        b /= g;
-    }
-}
-
-template <class T1, class T2,
-    class = std::enable_if_t <
-        std::is_unsigned<T1>::value &&
-        sizeof(T1) <= sizeof(std::uint64_t) >,
-    class = std::enable_if_t <
-        std::is_unsigned<T2>::value &&
-        sizeof(T2) <= sizeof(std::uint64_t) >
->
-void lowestTerms(T1& a, FeeUnit<T2>& b)
-{
-    if (auto const g = gcd(a, b.value()))
-    {
-        a /= g;
-        b /= g;
-    }
-}
-
-// Normally, these types wouldn't be swappable. For this file only, it's ok.
-template<class T1, class T2>
-static
-void
-maybe_swap (FeeUnit<T1>& lhs, Drops<T2>& rhs)
-{
-    if (lhs.value() < rhs.value())
-    {
-        Drops<T2> tmp = lhs.value() * units::drops;
-        lhs = rhs.value() * units::feeunits;
-        rhs = tmp;
-    }
-    // double check
-    assert(lhs.value() >= rhs.value());
-}
-
 // Scale using load as well as base rate
 Drops64
 scaleFeeLoad(FeeUnit64 fee, LoadFeeTrack const& feeTrack,
     Fees const& fees, bool bUnlimited)
 {
+    auto lowestTerms1 = [](auto& a, auto& b)
+    {
+        if (auto const g = gcd(a, b.value()))
+        {
+            a /= g;
+            b /= g;
+        }
+    };
+
+    // Normally, types with different units wouldn't be mathematically
+    // compatible. For this file only, it's ok.
+    auto lowestTerms2 = [](auto& a, auto& b)
+    {
+        if (auto const g = gcd(a.value(), b.value()))
+        {
+            a /= g;
+            b /= g;
+        }
+    };
+
+    // Normally, these types wouldn't be swappable. For this file only, it's ok.
+    auto maybe_swap = [](auto& lhs, auto& rhs)
+    {
+        if (lhs.value() < rhs.value())
+        {
+            auto tmp = lhs.value() * units::drops;
+            lhs = rhs.value() * units::feeunits;
+            rhs = tmp;
+        }
+        // double check
+        assert(lhs.value() >= rhs.value());
+    };
+
     if (fee == 0 * units::feeunit)
         return 0;
     std::uint32_t feeFactor;
@@ -226,9 +170,9 @@ scaleFeeLoad(FeeUnit64 fee, LoadFeeTrack const& feeTrack,
         * safe_cast<std::uint64_t>(feeTrack.getLoadBase());
     // Reduce fee * baseFee * feeFactor / (fees.units * lftNormalFee)
     // to lowest terms.
-    lowestTerms(fee, den);
-    lowestTerms(baseFee, den);
-    lowestTerms(feeFactor, den);
+    lowestTerms2(fee, den);
+    lowestTerms2(baseFee, den);
+    lowestTerms1(feeFactor, den);
 
     // fee and baseFee are 64 bit, feeFactor is 32 bit
     // Order fee and baseFee largest first

diff --git a/src/ripple/basics/feeunits.h b/src/ripple/basics/feeunits.h
@@ -68,11 +68,7 @@ operator<<(std::basic_ostream<Char, Traits>& os,
 template<class Unit, class T>
 std::string to_string(const quantity<Unit, T>& t)
 {
-    std::stringstream sstr;
-
-    sstr << t;
-
-    return sstr.str();
+    return std::to_string(t.value());
 }
 
 } // units
@@ -319,7 +315,7 @@ namespace detail
 // Need to cap to uint64 to uint32 due to JSON limitations
 template<class Unit>
 boost::units::quantity<Unit, std::uint32_t>
-clamp(boost::units::quantity<Unit, std::uint64_t> v)
+clamp32(boost::units::quantity<Unit, std::uint64_t> v)
 {
     constexpr Unit unit;
     constexpr boost::units::quantity<Unit, std::uint64_t> max32 =
@@ -349,7 +345,7 @@ Json::Value
 maybe_toJson(boost::units::quantity<Unit, T> src,
     std::false_type)
 {
-    return maybe_toJson(clamp(src), std::true_type{});
+    return maybe_toJson(clamp32(src), std::true_type{});
 }
 
 }