From b8f7fa0318d726faf6badd24d2974b705e83c1b9 Mon Sep 17 00:00:00 2001
From: Edward Hennis <ed@ripple.com>
Date: Thu, 23 May 2019 12:04:05 -0400
Subject: [PATCH] [FOLD] Review feedback from @mellery451:

* Convert scaleFeeLoad worker functions to lambdas.
* Simplify to_string(quantity).
* Rename detail::clamp to detail::clamp32.
---
 src/ripple/app/misc/impl/LoadFeeTrack.cpp | 130 ++++++----------------
 src/ripple/basics/feeunits.h              |  10 +-
 2 files changed, 40 insertions(+), 100 deletions(-)

diff --git a/src/ripple/app/misc/impl/LoadFeeTrack.cpp b/src/ripple/app/misc/impl/LoadFeeTrack.cpp
index 153f26c953a..5a31e7f153c 100644
--- 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
index 9795ef8bde8..b363e58f642 100644
--- 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{});
 }
 
 }