From f7daa6da4dde66f2c70683dd40469093de51c8a4 Mon Sep 17 00:00:00 2001 From: Long Le Date: Mon, 6 Feb 2023 10:34:10 +0100 Subject: [PATCH] Use <=> operator for base_uint, Issue, and Book (#4411) Fixes #2525 --- .../app/tx/impl/details/NFTokenUtils.cpp | 11 ++- src/ripple/basics/base_uint.h | 86 ++++++------------- src/ripple/protocol/Book.h | 28 +++--- src/ripple/protocol/Issue.h | 36 ++++---- src/ripple/protocol/impl/Book.cpp | 54 ------------ src/ripple/protocol/impl/Issue.cpp | 56 ------------ src/test/basics/base_uint_test.cpp | 83 ++++++++++++++++-- 7 files changed, 138 insertions(+), 216 deletions(-) diff --git a/src/ripple/app/tx/impl/details/NFTokenUtils.cpp b/src/ripple/app/tx/impl/details/NFTokenUtils.cpp index d1214a98ee8..0625f63e550 100644 --- a/src/ripple/app/tx/impl/details/NFTokenUtils.cpp +++ b/src/ripple/app/tx/impl/details/NFTokenUtils.cpp @@ -146,19 +146,22 @@ getPageForToken( return nullptr; else { - // This would be an ideal place for the spaceship operator... - int const relation = compare(id & nft::pageMask, cmp); + auto const relation{(id & nft::pageMask) <=> cmp}; if (relation == 0) + { // If the passed in id belongs exactly on this (full) page // this account simply cannot store the NFT. return nullptr; + } - else if (relation > 0) + if (relation > 0) + { // We need to leave the entire contents of this page in // narr so carr stays empty. The new NFT will be // inserted in carr. This keeps the NFTs that must be // together all on their own page. splitIter = narr.end(); + } // If neither of those conditions apply then put all of // narr into carr and produce an empty narr where the new NFT @@ -228,7 +231,7 @@ compareTokens(uint256 const& a, uint256 const& b) // 96-bits are identical we still need a fully deterministic sort. // So we sort on the low 96-bits first. If those are equal we sort on // the whole thing. - if (auto const lowBitsCmp = compare(a & nft::pageMask, b & nft::pageMask); + if (auto const lowBitsCmp{(a & nft::pageMask) <=> (b & nft::pageMask)}; lowBitsCmp != 0) return lowBitsCmp < 0; diff --git a/src/ripple/basics/base_uint.h b/src/ripple/basics/base_uint.h index 8f277c3003c..8b15b082647 100644 --- a/src/ripple/basics/base_uint.h +++ b/src/ripple/basics/base_uint.h @@ -33,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -549,103 +550,66 @@ using uint160 = base_uint<160>; using uint256 = base_uint<256>; template -inline int -compare(base_uint const& a, base_uint const& b) +[[nodiscard]] inline constexpr std::strong_ordering +operator<=>(base_uint const& lhs, base_uint const& rhs) { - auto ret = std::mismatch(a.cbegin(), a.cend(), b.cbegin()); - - if (ret.first == a.cend()) - return 0; + // This comparison might seem wrong on a casual inspection because it + // compares data internally stored as std::uint32_t byte-by-byte. But + // note that the underlying data is stored in big endian, even if the + // plaform is little endian. This makes the comparison correct. + // + // FIXME: use std::lexicographical_compare_three_way once support is + // added to MacOS. - // a > b - if (*ret.first > *ret.second) - return 1; + auto const ret = std::mismatch(lhs.cbegin(), lhs.cend(), rhs.cbegin()); - // a < b - return -1; -} - -template -inline bool -operator<(base_uint const& a, base_uint const& b) -{ - return compare(a, b) < 0; -} + // a == b + if (ret.first == lhs.cend()) + return std::strong_ordering::equivalent; -template -inline bool -operator<=(base_uint const& a, base_uint const& b) -{ - return compare(a, b) <= 0; + return (*ret.first > *ret.second) ? std::strong_ordering::greater + : std::strong_ordering::less; } -template -inline bool -operator>(base_uint const& a, base_uint const& b) +template +[[nodiscard]] inline constexpr bool +operator==(base_uint const& lhs, base_uint const& rhs) { - return compare(a, b) > 0; -} - -template -inline bool -operator>=(base_uint const& a, base_uint const& b) -{ - return compare(a, b) >= 0; -} - -template -inline bool -operator==(base_uint const& a, base_uint const& b) -{ - return compare(a, b) == 0; -} - -template -inline bool -operator!=(base_uint const& a, base_uint const& b) -{ - return compare(a, b) != 0; + return (lhs <=> rhs) == 0; } //------------------------------------------------------------------------------ template -inline bool +inline constexpr bool operator==(base_uint const& a, std::uint64_t b) { return a == base_uint(b); } -template -inline bool -operator!=(base_uint const& a, std::uint64_t b) -{ - return !(a == b); -} - //------------------------------------------------------------------------------ template -inline const base_uint +inline constexpr base_uint operator^(base_uint const& a, base_uint const& b) { return base_uint(a) ^= b; } template -inline const base_uint +inline constexpr base_uint operator&(base_uint const& a, base_uint const& b) { return base_uint(a) &= b; } template -inline const base_uint +inline constexpr base_uint operator|(base_uint const& a, base_uint const& b) { return base_uint(a) |= b; } template -inline const base_uint +inline constexpr base_uint operator+(base_uint const& a, base_uint const& b) { return base_uint(a) += b; diff --git a/src/ripple/protocol/Book.h b/src/ripple/protocol/Book.h index 1469b60dd1b..609989062c0 100644 --- a/src/ripple/protocol/Book.h +++ b/src/ripple/protocol/Book.h @@ -65,28 +65,24 @@ hash_append(Hasher& h, Book const& b) Book reversed(Book const& book); -/** Ordered comparison. */ -int -compare(Book const& lhs, Book const& rhs); - /** Equality comparison. */ /** @{ */ -bool -operator==(Book const& lhs, Book const& rhs); -bool -operator!=(Book const& lhs, Book const& rhs); +[[nodiscard]] inline constexpr bool +operator==(Book const& lhs, Book const& rhs) +{ + return (lhs.in == rhs.in) && (lhs.out == rhs.out); +} /** @} */ /** Strict weak ordering. */ /** @{ */ -bool -operator<(Book const& lhs, Book const& rhs); -bool -operator>(Book const& lhs, Book const& rhs); -bool -operator>=(Book const& lhs, Book const& rhs); -bool -operator<=(Book const& lhs, Book const& rhs); +[[nodiscard]] inline constexpr std::weak_ordering +operator<=>(Book const& lhs, Book const& rhs) +{ + if (auto const c{lhs.in <=> rhs.in}; c != 0) + return c; + return lhs.out <=> rhs.out; +} /** @} */ } // namespace ripple diff --git a/src/ripple/protocol/Issue.h b/src/ripple/protocol/Issue.h index 11c45c0136c..be7733677a1 100644 --- a/src/ripple/protocol/Issue.h +++ b/src/ripple/protocol/Issue.h @@ -63,31 +63,29 @@ hash_append(Hasher& h, Issue const& r) hash_append(h, r.currency, r.account); } -/** Ordered comparison. - The assets are ordered first by currency and then by account, - if the currency is not XRP. -*/ -int -compare(Issue const& lhs, Issue const& rhs); - /** Equality comparison. */ /** @{ */ -bool -operator==(Issue const& lhs, Issue const& rhs); -bool -operator!=(Issue const& lhs, Issue const& rhs); +[[nodiscard]] inline constexpr bool +operator==(Issue const& lhs, Issue const& rhs) +{ + return (lhs.currency == rhs.currency) && + (isXRP(lhs.currency) || lhs.account == rhs.account); +} /** @} */ /** Strict weak ordering. */ /** @{ */ -bool -operator<(Issue const& lhs, Issue const& rhs); -bool -operator>(Issue const& lhs, Issue const& rhs); -bool -operator>=(Issue const& lhs, Issue const& rhs); -bool -operator<=(Issue const& lhs, Issue const& rhs); +[[nodiscard]] inline constexpr std::weak_ordering +operator<=>(Issue const& lhs, Issue const& rhs) +{ + if (auto const c{lhs.currency <=> rhs.currency}; c != 0) + return c; + + if (isXRP(lhs.currency)) + return std::weak_ordering::equivalent; + + return (lhs.account <=> rhs.account); +} /** @} */ //------------------------------------------------------------------------------ diff --git a/src/ripple/protocol/impl/Book.cpp b/src/ripple/protocol/impl/Book.cpp index 323985e6114..3ad22675d1b 100644 --- a/src/ripple/protocol/impl/Book.cpp +++ b/src/ripple/protocol/impl/Book.cpp @@ -47,58 +47,4 @@ reversed(Book const& book) return Book(book.out, book.in); } -/** Ordered comparison. */ -int -compare(Book const& lhs, Book const& rhs) -{ - int const diff(compare(lhs.in, rhs.in)); - if (diff != 0) - return diff; - return compare(lhs.out, rhs.out); -} - -/** Equality comparison. */ -/** @{ */ -bool -operator==(Book const& lhs, Book const& rhs) -{ - return (lhs.in == rhs.in) && (lhs.out == rhs.out); -} - -bool -operator!=(Book const& lhs, Book const& rhs) -{ - return (lhs.in != rhs.in) || (lhs.out != rhs.out); -} -/** @} */ - -/** Strict weak ordering. */ -/** @{ */ -bool -operator<(Book const& lhs, Book const& rhs) -{ - int const diff(compare(lhs.in, rhs.in)); - if (diff != 0) - return diff < 0; - return lhs.out < rhs.out; -} - -bool -operator>(Book const& lhs, Book const& rhs) -{ - return rhs < lhs; -} - -bool -operator>=(Book const& lhs, Book const& rhs) -{ - return !(lhs < rhs); -} - -bool -operator<=(Book const& lhs, Book const& rhs) -{ - return !(rhs < lhs); -} - } // namespace ripple diff --git a/src/ripple/protocol/impl/Issue.cpp b/src/ripple/protocol/impl/Issue.cpp index 24a8c764efb..e727cb4cade 100644 --- a/src/ripple/protocol/impl/Issue.cpp +++ b/src/ripple/protocol/impl/Issue.cpp @@ -43,60 +43,4 @@ operator<<(std::ostream& os, Issue const& x) return os; } -/** Ordered comparison. - The assets are ordered first by currency and then by account, - if the currency is not XRP. -*/ -int -compare(Issue const& lhs, Issue const& rhs) -{ - int diff = compare(lhs.currency, rhs.currency); - if (diff != 0) - return diff; - if (isXRP(lhs.currency)) - return 0; - return compare(lhs.account, rhs.account); -} - -/** Equality comparison. */ -/** @{ */ -bool -operator==(Issue const& lhs, Issue const& rhs) -{ - return compare(lhs, rhs) == 0; -} - -bool -operator!=(Issue const& lhs, Issue const& rhs) -{ - return !(lhs == rhs); -} -/** @} */ - -/** Strict weak ordering. */ -/** @{ */ -bool -operator<(Issue const& lhs, Issue const& rhs) -{ - return compare(lhs, rhs) < 0; -} - -bool -operator>(Issue const& lhs, Issue const& rhs) -{ - return rhs < lhs; -} - -bool -operator>=(Issue const& lhs, Issue const& rhs) -{ - return !(lhs < rhs); -} - -bool -operator<=(Issue const& lhs, Issue const& rhs) -{ - return !(rhs < lhs); -} - } // namespace ripple diff --git a/src/test/basics/base_uint_test.cpp b/src/test/basics/base_uint_test.cpp index c1ba7302ae8..9b1f7696dd5 100644 --- a/src/test/basics/base_uint_test.cpp +++ b/src/test/basics/base_uint_test.cpp @@ -57,8 +57,76 @@ struct nonhash struct base_uint_test : beast::unit_test::suite { using test96 = base_uint<96>; - static_assert(std::is_copy_constructible::value, ""); - static_assert(std::is_copy_assignable::value, ""); + static_assert(std::is_copy_constructible::value); + static_assert(std::is_copy_assignable::value); + + void + testComparisons() + { + { + static constexpr std:: + array, 6> + test_args{ + {{"0000000000000000", "0000000000000001"}, + {"0000000000000000", "ffffffffffffffff"}, + {"1234567812345678", "2345678923456789"}, + {"8000000000000000", "8000000000000001"}, + {"aaaaaaaaaaaaaaa9", "aaaaaaaaaaaaaaaa"}, + {"fffffffffffffffe", "ffffffffffffffff"}}}; + + for (auto const& arg : test_args) + { + ripple::base_uint<64> const u{arg.first}, v{arg.second}; + BEAST_EXPECT(u < v); + BEAST_EXPECT(u <= v); + BEAST_EXPECT(u != v); + BEAST_EXPECT(!(u == v)); + BEAST_EXPECT(!(u > v)); + BEAST_EXPECT(!(u >= v)); + BEAST_EXPECT(!(v < u)); + BEAST_EXPECT(!(v <= u)); + BEAST_EXPECT(v != u); + BEAST_EXPECT(!(v == u)); + BEAST_EXPECT(v > u); + BEAST_EXPECT(v >= u); + BEAST_EXPECT(u == u); + BEAST_EXPECT(v == v); + } + } + + { + static constexpr std::array< + std::pair, + 6> + test_args{{ + {"000000000000000000000000", "000000000000000000000001"}, + {"000000000000000000000000", "ffffffffffffffffffffffff"}, + {"0123456789ab0123456789ab", "123456789abc123456789abc"}, + {"555555555555555555555555", "55555555555a555555555555"}, + {"aaaaaaaaaaaaaaa9aaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaa"}, + {"fffffffffffffffffffffffe", "ffffffffffffffffffffffff"}, + }}; + + for (auto const& arg : test_args) + { + ripple::base_uint<96> const u{arg.first}, v{arg.second}; + BEAST_EXPECT(u < v); + BEAST_EXPECT(u <= v); + BEAST_EXPECT(u != v); + BEAST_EXPECT(!(u == v)); + BEAST_EXPECT(!(u > v)); + BEAST_EXPECT(!(u >= v)); + BEAST_EXPECT(!(v < u)); + BEAST_EXPECT(!(v <= u)); + BEAST_EXPECT(v != u); + BEAST_EXPECT(!(v == u)); + BEAST_EXPECT(v > u); + BEAST_EXPECT(v >= u); + BEAST_EXPECT(u == u); + BEAST_EXPECT(v == v); + } + } + } void run() override @@ -66,9 +134,12 @@ struct base_uint_test : beast::unit_test::suite testcase("base_uint: general purpose tests"); static_assert( - !std::is_constructible>::value, ""); + !std::is_constructible>::value); static_assert( - !std::is_assignable>::value, ""); + !std::is_assignable>::value); + + testComparisons(); + // used to verify set insertion (hashing required) std::unordered_set> uset; @@ -112,8 +183,8 @@ struct base_uint_test : beast::unit_test::suite BEAST_EXPECT(d == --t); } - BEAST_EXPECT(compare(u, v) < 0); - BEAST_EXPECT(compare(v, u) > 0); + BEAST_EXPECT(u < v); + BEAST_EXPECT(v > u); v = u; BEAST_EXPECT(v == u);