Skip to content

Commit

Permalink
[FOLD] Clean up conversion code for XRPAmount and TaggedFee:
Browse files Browse the repository at this point in the history 
* Remove all converting constructors in favor of a more general `as<>`
  function that returns the desired type, using `safe_cast` or range
  checking and `static_cast`, as appropriate.
  • Loading branch information
@ximinez
ximinez committed Aug 20, 2019
1 parent befd3bd commit 404fdf6
Show file tree
Hide file tree
Showing 5 changed files with 164 additions and 46 deletions.
7 changes: 3 additions & 4 deletions src/ripple/app/misc/FeeVoteImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,16 +155,15 @@ FeeVoteImpl::doVoting(
assert ((lastClosedLedger->info().seq % 256) == 0);

detail::VotableInteger<std::uint64_t> baseFeeVote (
static_cast<XRPAmountU64>(lastClosedLedger->fees().base),
lastClosedLedger->fees().base.as<XRPAmountU64>(),
target_.reference_fee);

detail::VotableInteger<std::uint32_t> baseReserveVote(
static_cast<XRPAmountU32>(
lastClosedLedger->fees().accountReserve(0)),
lastClosedLedger->fees().accountReserve(0).as<XRPAmountU32>(),
target_.account_reserve);

detail::VotableInteger<std::uint32_t> incReserveVote (
static_cast<XRPAmountU32>(lastClosedLedger->fees().increment),
lastClosedLedger->fees().increment.as<XRPAmountU32>(),
target_.owner_reserve);

for (auto const& val : set)
Expand Down
50 changes: 34 additions & 16 deletions src/ripple/basics/XRPAmount.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,24 +97,15 @@ class XRPAmountBase
return *this;
}

/** This constructor is intended to only be used by
explicit static_cast operations */
template <class Other,
class = enable_if_compatible_t <Other> >
explicit
/** Instances with a type that is "safe" to covert
to this one can be converted implicitly */
template <class Other, class = std::enable_if_t<
is_compatible_v <Other> &&
is_safetocasttovalue_v <value_type, Other> >>
constexpr
XRPAmountBase (XRPAmountBase<Other> drops)
: drops_ (static_cast<value_type> (drops.drops()))
XRPAmountBase(XRPAmountBase<Other> const& xrp)
: XRPAmountBase (safe_cast<value_type> (xrp.drops()))
{
static_assert(!is_safetocasttovalue_v<value_type, Other>,
"This is a safe conversion");
if ((drops_ > std::numeric_limits<value_type>::max()) ||
(!std::numeric_limits<value_type>::is_signed && drops_ < 0) ||
(std::numeric_limits<value_type>::is_signed &&
drops_ < std::numeric_limits<value_type>::lowest()))
{
Throw<std::runtime_error>("XRPAmount conversion out of range");
}
}

constexpr
Expand Down Expand Up @@ -192,6 +183,8 @@ class XRPAmountBase
XRPAmountBase
operator- () const
{
static_assert( std::is_signed_v<T>,
"- operator illegal on unsigned XRPAmount types");
return { -drops_ };
}

Expand Down Expand Up @@ -257,6 +250,31 @@ class XRPAmountBase
double
decimalXRP () const;

template <class Dest,
class = std::enable_if_t<
std::is_same_v<typename Dest::unit_type, typename unit_type> &&
is_compatible_v <typename Dest::value_type> > >
Dest
as() const
{
using desttype = Dest::value_type;
if constexpr (is_safetocasttovalue_v <desttype, value_type>)
{
return { safe_cast<desttype>(fee_) };
}
else
{
if ((drops_ > std::numeric_limits<desttype>::max()) ||
(!std::numeric_limits<desttype>::is_signed && drops_ < 0) ||
(std::numeric_limits<desttype>::is_signed &&
drops_ < std::numeric_limits<desttype>::lowest()))
{
Throw<std::runtime_error>("XRPAmount conversion out of range");
}
return { static_cast<desttype>(drops_) };
}
}

Json::Value
json () const
{
Expand Down
46 changes: 24 additions & 22 deletions src/ripple/basics/feeunits.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,7 @@ class TaggedFee
protected:
template<class Other>
static constexpr bool is_compatible_v =
((std::is_integral_v<Other> && std::is_integral_v<value_type>) ||
std::is_floating_point_v<value_type> ) &&
std::is_arithmetic_v<Other> && std::is_arithmetic_v<value_type> &&
std::is_convertible_v<Other, value_type>;

template<class OtherFee, class = enable_if_unit_t<OtherFee> >
Expand Down Expand Up @@ -146,26 +145,6 @@ class TaggedFee
{
}

/** Instances with the same unit, but a type that is
not "safe" to covert to this one must be converted
explicitly */
template <class Other, class = std::enable_if_t<
is_compatible_v <Other> &&
!is_safetocasttovalue_v <value_type, Other> >>
explicit
constexpr
TaggedFee(TaggedFee<unit_type, Other> fee)
: fee_ (static_cast<value_type> (fee.fee()))
{
if ((fee_ > std::numeric_limits<value_type>::max()) ||
(!std::numeric_limits<value_type>::is_signed && fee_ < 0) ||
(std::numeric_limits<value_type>::is_signed &&
fee_ < std::numeric_limits<value_type>::lowest()))
{
Throw<std::runtime_error>("Fee conversion out of range");
}
}

constexpr
TaggedFee
operator* (value_type const& rhs) const
Expand Down Expand Up @@ -314,6 +293,29 @@ class TaggedFee
return static_cast<double>(fee_) / reference.fee();
}

template <class Dest,
class = enable_if_compatiblefee_t<Dest> >
Dest
as() const
{
using desttype = Dest::value_type;
if constexpr (is_safetocasttovalue_v <desttype, value_type>)
{
return Dest{ safe_cast<desttype>(fee_) };
}
else
{
if ((fee_ > std::numeric_limits<desttype>::max()) ||
(!std::numeric_limits<desttype>::is_signed && fee_ < 0) ||
(std::numeric_limits<desttype>::is_signed &&
fee_ < std::numeric_limits<desttype>::lowest()))
{
Throw<std::runtime_error>("Fee conversion out of range");
}
return Dest{ static_cast<desttype>(fee_) };
}
}

// TODO: Rewrite this to use `if constexpr` once we move to C++17.
//
// `is_usable_unit_v` is checked to ensure that only values with
Expand Down
96 changes: 96 additions & 0 deletions src/test/basics/XRPAmount_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,101 @@ class XRPAmount_test : public beast::unit_test::suite
BEAST_EXPECT(test.decimalXRP() == -100);
}

void testFunctions()
{
// Explicitly test every defined function for the TaggedFee class
// since some of them are templated, but not used anywhere else.
auto make = [&](auto x) -> XRPAmount {
return x; };
auto explicitmake = [&](auto x) -> XRPAmount {
return XRPAmount{ x };
};

XRPAmount defaulted;
(void)defaulted;
XRPAmount test{ 0 };
BEAST_EXPECT(test.drops() == 0);

test = make(beast::zero);
BEAST_EXPECT(test.drops() == 0);

test = beast::zero;
BEAST_EXPECT(test.drops() == 0);

test = make(100);
BEAST_EXPECT(test.drops() == 100);

test = make(100u);
BEAST_EXPECT(test.drops() == 100);

XRPAmount const targetSame{ 200u };
XRPAmountU32 const targetOther{ 300u };
test = make(targetSame);
BEAST_EXPECT(test.drops() == 200);
BEAST_EXPECT(test == targetSame);
BEAST_EXPECT(test < XRPAmount{ 1000 });
BEAST_EXPECT(test > XRPAmount{ 100 });
test = targetOther.as<XRPAmount>();
BEAST_EXPECT(test.drops() == 300);
BEAST_EXPECT(test == targetOther);

test = std::int64_t(200);
BEAST_EXPECT(test.drops() == 200);
test = std::uint32_t(300);
BEAST_EXPECT(test.drops() == 300);

test = targetSame;
BEAST_EXPECT(test.drops() == 200);
test = targetOther.as<XRPAmount>();
BEAST_EXPECT(test.drops() == 300);
BEAST_EXPECT(test == targetOther);
auto testOther = test.as<XRPAmountU32>();
BEAST_EXPECT(testOther.drops() == 300);

test = targetSame * 2;
BEAST_EXPECT(test.drops() == 400);
test = 3 * targetSame;
BEAST_EXPECT(test.drops() == 600);
test = targetSame / 10;
BEAST_EXPECT(test.drops() == 20);

test += targetSame;
BEAST_EXPECT(test.drops() == 220);

test -= targetSame;
BEAST_EXPECT(test.drops() == 20);

test++;
BEAST_EXPECT(test.drops() == 21);
++test;
BEAST_EXPECT(test.drops() == 22);
test--;
BEAST_EXPECT(test.drops() == 21);
--test;
BEAST_EXPECT(test.drops() == 20);

test *= 5;
BEAST_EXPECT(test.drops() == 100);
test /= 2;
BEAST_EXPECT(test.drops() == 50);
test %= 13;
BEAST_EXPECT(test.drops() == 11);

// legal with signed
test = -test;
BEAST_EXPECT(test.drops() == -11);
BEAST_EXPECT(test.signum() == -1);
BEAST_EXPECT(to_string(test) == "-11");

BEAST_EXPECT(test);
test = 0;
BEAST_EXPECT(!test);
BEAST_EXPECT(test.signum() == 0);
test = targetSame;
BEAST_EXPECT(test.signum() == 1);
BEAST_EXPECT(to_string(test) == "200");
}

void testMulRatio()
{
testcase ("mulRatio");
Expand Down Expand Up @@ -237,6 +332,7 @@ class XRPAmount_test : public beast::unit_test::suite
testComparisons ();
testAddSub ();
testDecimal ();
testFunctions ();
testMulRatio ();
}
};
Expand Down
11 changes: 7 additions & 4 deletions src/test/basics/feeunits_test.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,11 @@ class feeunits_test

test = targetSame;
BEAST_EXPECT(test.fee() == 200);
test = safe_cast<FeeUnit64>(targetOther);
test = targetOther.as<FeeUnit64>();
BEAST_EXPECT(test.fee() == 300);
BEAST_EXPECT(test == targetOther);
auto testOther = test.as<FeeUnit32>();
BEAST_EXPECT(testOther.fee() == 300);

test = targetSame * 2;
BEAST_EXPECT(test.fee() == 400);
Expand Down Expand Up @@ -280,9 +283,11 @@ class feeunits_test
BEAST_EXPECT(test == targetSame);
BEAST_EXPECT(test < FeeLevelDouble{ 1000.0 });
BEAST_EXPECT(test > FeeLevelDouble{ 100.0 });
test = explicitmake(targetOther);
test = targetOther.as<FeeLevelDouble>();
BEAST_EXPECT(test.fee() == 300);
BEAST_EXPECT(test == targetOther);
auto testOther = test.as<FeeLevel64>();
BEAST_EXPECT(testOther.fee() == 300);

test = 200.0;
BEAST_EXPECT(test.fee() == 200);
Expand All @@ -291,8 +296,6 @@ class feeunits_test

test = targetSame;
BEAST_EXPECT(test.fee() == 200);
test = static_cast<FeeLevelDouble>(targetOther);
BEAST_EXPECT(test.fee() == 300);

test = targetSame * 2;
BEAST_EXPECT(test.fee() == 400);
Expand Down

0 comments on commit 404fdf6

Please sign in to comment.