Use <=> operator for base_uint, Issue, and Book: #4411

drlongle · 2023-02-07T10:25:03Z

High Level Overview of Change

This PR resolves issue 2525. The PR implements the operator==and the operator<=> (aka the spaceship operator) in base_uint, Issue, and Book. C++20-compliant compilers automatically provide the remaining comparison operators (e.g. operator<, operator<=, ...). I also remove the function compare() because it is no longer needed. The PR provides the same semantics as the existing code. All unit tests pass. I also added some unit tests to gain further confidence.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Technical debt
Refactor (non-breaking change that only restructures code)
Tests (You added tests for code that already exists, or your new feature included in this PR)
Documentation Updates
Release

drlongle · 2023-02-07T11:37:44Z

It seems that std::lexicographical_compare_three_way is not yet supported by Xcode. I'll look into alternatives.

drlongle · 2023-02-08T12:48:01Z

PR is ready for review. It would be nice to use std::is_eq(), std::is_neq() and std::lexicographical_compare_three_way(). Unfortunately, these functions are not yet suppored on MacOS. I use std::weak_ordering::equivalent and std::mismatch() instead.

nbougalis

Nice. I think the code can be further improved and cleaned up. I left some comments.

src/ripple/basics/base_uint.h

nbougalis · 2023-02-08T22:57:59Z

src/ripple/basics/base_uint.h

+template <std::size_t Bits, typename Tag>
+class base_uint;
+
+template <std::size_t Bits, typename Tag>
+[[nodiscard]] constexpr bool
+operator==(base_uint<Bits, Tag> const& lhs, base_uint<Bits, Tag> const& rhs);
+


All this is unnecessary.

My intention was to be able to compare the arrays data_ with std::uint32_t data elements instead of using std::mismatch() with unsigned char data elements. It's probably an overkill. I revert this change and take your suggested code.

Ah, I see what you were trying to do. It makes sense, but you need to be careful (and you were). This trick will ONLY work for equality/inequality comparisons, because the data_ array is stored in big endian order, which on platforms like the x86 is NOT the native endian order. Relational comparisons (gt/gte/lt/lte) break. Consider, for example, the following two 32-bit unsigned values:

a = 256, b = 1

If you convert them into big endian they look like

x = 65536, y = 16777216

Notice what happened... a > b but x < y. Oops!

I think that having the "bespoke" operator== is fine although I doubt there are any performance gains to be had. But you should definitely consider adding a comment explaining that this is safe, despite the fact that the individual limbs (the entries in the data_ array) are in big endian, because we are comparing for equality/inequality.

The existing compare function CANNOT do that trick and MUST use std::mismatch. It could also use a comment too, explaining that this works because the pointer returned from cbegin is a byte-based view of the data_ which is actually in big-endian order.

You are right. This is quite tricky. I use std::mismatch for now.

src/ripple/basics/base_uint.h

nbougalis · 2023-02-08T23:11:54Z

src/ripple/protocol/Book.h

+[[nodiscard]] bool
 operator==(Book const& lhs, Book const& rhs);
-bool
-operator!=(Book const& lhs, Book const& rhs);
 /** @} */

 /** 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]] std::weak_ordering
+operator<=>(Book const& lhs, Book const& rhs);


I think you can just use defaulted comparisons for these. Simply add the following inside the declaration of Book as a public and you automatically get all six relational operators:

auto operator<=>(Bool const&) const = default;

NOTE, HOWEVER, THAT THIS HAS A RISK:* first, the order of the member variables matters (since the default comparison goes through each member in order) and if any additional variables are added, it might cause subtle breakage of comparisons.

With that in mind, I'm not sure it's worth defaulting, but just pointing it out.

You are right. We could default here but the operator<=> becomes a bit fragile as you pointed out.

I would not default unless you insist. Please let me know.

src/ripple/protocol/impl/Issue.cpp

nbougalis

Looks good. Left some small comments.

src/ripple/app/tx/impl/details/NFTokenUtils.cpp

src/ripple/basics/base_uint.h

src/ripple/protocol/impl/Book.cpp

src/ripple/protocol/impl/Issue.cpp

scottschurr

Nice work! As far as I can see everything should work correctly.

I left a number of comments for things I think might be minor improvements. Specifically, I think you're working too hard when you use the stored result of the <=> operator. You don't need to use constants like std::weak_ordering::equivalent. It is sufficient (and easier to read) to compare to literal 0.

Comparison to 0 is how the compiler rewrites comparison expressions to use the <=> operator. See section 2.2.3 of the original spaceship paper: https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0515r3.pdf

I think that's the most important change to consider. I'm also open to being told I'm wrong. But I think it's useful to get this right early on, since this is the first introduction of the <=> operator to this code base. People will be copying the example that you set here.

scottschurr · 2023-02-15T01:33:59Z

src/ripple/app/tx/impl/details/NFTokenUtils.cpp

+                // FIXME: use std::is_eq and std::is_gt once support is
+                //        added to MacOS.
+                auto const relation{(id & nft::pageMask) <=> cmp};
+                if (relation == std::strong_ordering::equivalent)


This works. So no change necessary. But I think you're working harder than you need to. Consider the rewrite rules that C++ follows with the <=> operator:

(a <=> b) @ 0

So I don't think we need std::strong_ordering::equivalent or std::is_eq or std::is_gt. All we need is 0. I suggest rewriting the comparison like this:

auto const relation{(id & nft::pageMask) <=> cmp}; if (relation == 0)

I think that allows you to remove the FIXME. I think it's also easier to read.

Thanks for this suggestion. It makes the code clean and concise.

scottschurr · 2023-02-15T01:35:10Z

src/ripple/app/tx/impl/details/NFTokenUtils.cpp


-                else if (relation > 0)
+                if (relation == std::strong_ordering::greater)


Similar to the above comment, you can simplify this to

if (relation > 0)

with no loss of generality.

Thanks. Fixed.

scottschurr · 2023-02-15T01:38:03Z

src/ripple/app/tx/impl/details/NFTokenUtils.cpp

+    // FIXME: use std::is_eq and std::is_lt once support is
+    //        added to MacOS.
+    if (auto const c{(a & nft::pageMask) <=> (b & nft::pageMask)};
+        c != std::strong_ordering::equivalent)
+        return c == std::strong_ordering::less;


Again, this works, but I think you're working harder than necessary. A more minimal change might look like this:

if (auto const lowBitsCmp = (a & nft::pageMask) <=> (b & nft::pageMask); lowBitsCmp != 0) return lowBitsCmp < 0;

No FIXME comment necessary.

Thanks. Fixed.

scottschurr · 2023-02-15T01:38:50Z

src/ripple/basics/base_uint.h

@@ -549,103 +550,77 @@ using uint160 = base_uint<160>;
 using uint256 = base_uint<256>;

 template <std::size_t Bits, class Tag>
-inline int
-compare(base_uint<Bits, Tag> const& a, base_uint<Bits, Tag> const& b)
+[[nodiscard]] inline constexpr std::strong_ordering


I really like the [[nodiscard]] and constexpr. Nice!

scottschurr · 2023-02-15T01:44:29Z

src/ripple/basics/base_uint.h

    // a > b
    if (*ret.first > *ret.second)
-        return 1;
+        return std::strong_ordering::greater;

    // a < b
-    return -1;
-}
-
-template <std::size_t Bits, class Tag>
-inline bool
-operator<(base_uint<Bits, Tag> const& a, base_uint<Bits, Tag> const& b)
-{
-    return compare(a, b) < 0;
-}
-
-template <std::size_t Bits, class Tag>
-inline bool
-operator<=(base_uint<Bits, Tag> const& a, base_uint<Bits, Tag> const& b)
-{
-    return compare(a, b) <= 0;
-}
-
-template <std::size_t Bits, class Tag>
-inline bool
-operator>(base_uint<Bits, Tag> const& a, base_uint<Bits, Tag> const& b)
-{
-    return compare(a, b) > 0;
-}
-
-template <std::size_t Bits, class Tag>
-inline bool
-operator>=(base_uint<Bits, Tag> const& a, base_uint<Bits, Tag> const& b)
-{
-    return compare(a, b) >= 0;
-}
-
-template <std::size_t Bits, class Tag>
-inline bool
-operator==(base_uint<Bits, Tag> const& a, base_uint<Bits, Tag> const& b)
-{
-    return compare(a, b) == 0;
+    return std::strong_ordering::less;


This works fine. If you want you could reduce it a bit like this:

return (*ret.first > *ret.second) ? std::strong_ordering::greater : std::strong_ordering::less;

That reduces from two returns to one. But your choice.

Thanks. Fixed.

Wait what? You can’t reduce lines 571-575 down to a single return.

Am I not understanding the suggestion here?

The code is now like below.

auto const ret = std::mismatch(lhs.cbegin(), lhs.cend(), rhs.cbegin()); // a == b if (ret.first == lhs.cend()) return std::strong_ordering::equivalent; return (*ret.first > *ret.second) ? std::strong_ordering::greater : std::strong_ordering::less;

We can use a ternary operator because we know that a != b. If you want to revert this change, let me know.

scottschurr · 2023-02-15T02:24:53Z

src/test/basics/base_uint_test.cpp

+                {"123456789abc", "aaaaaaaaaaaa"},
+                {"999999999999", "aaaaaaaaaaaa"}};
+
+            for (auto& arg : test_args)


Could be auto const& arg.

Thanks. Fixed.

scottschurr · 2023-02-15T02:26:50Z

src/test/basics/base_uint_test.cpp

+
+            for (auto& arg : test_args)
+            {
+                ripple::base_uint<96> u{arg.first}, v{arg.second};


Could be

ripple::base_uint<96> const u{arg.first}, v{arg.second};

Thanks. Fixed.

scottschurr · 2023-02-15T02:27:22Z

src/test/basics/base_uint_test.cpp

+            std::vector<std::pair<std::string, std::string>> test_args{
+                {"0123456789ab", "123456789abc"},
+                {"010101010101", "111111111111"},
+                {"000000000000", "010101010101"},
+                {"123456789abc", "aaaaaaaaaaaa"},
+                {"999999999999", "aaaaaaaaaaaa"}};


Again, we're only exercising half the bits in the uint_base<96>. Consider this alternative:

static constexpr std::array< std::pair<std::string_view, std::string_view>, 6> test_args{{ {"000000000000000000000000", "000000000000000000000001"}, {"000000000000000000000000", "ffffffffffffffffffffffff"}, {"0123456789ab0123456789ab", "123456789abc123456789abc"}, {"555555555555555555555555", "55555555555a555555555555"}, {"aaaaaaaaaaaaaaa9aaaaaaaa", "aaaaaaaaaaaaaaaaaaaaaaaa"}, {"fffffffffffffffffffffffe", "ffffffffffffffffffffffff"}, }};

Thanks. Fixed.

scottschurr · 2023-02-15T02:28:50Z

src/ripple/basics/base_uint.h

+    // 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.


Nice comment!

scottschurr · 2023-02-15T15:39:14Z

src/test/basics/base_uint_test.cpp

    static_assert(std::is_copy_constructible<test96>::value, "");
    static_assert(std::is_copy_assignable<test96>::value, "");


These are pre-C++17 static_asserts. While you're in the file would you consider removing the closing , ""? Thanks.

Thanks. Fixed.

scottschurr

Looks good to me! I spotted one more possible change, but it's not critical. So I'm approving. Feel free to make that change if you want, even with this approval.

src/ripple/protocol/Book.h

src/test/basics/base_uint_test.cpp

scottschurr · 2023-02-16T19:06:03Z

When you squash all of these commits, try to keep the first line of the commit message under 60 characters or so. You might consider something like this for your commit message:

Use <=> operator for base_uint, Issue, and Book (#4411):

Fixes #2525.

scottschurr

👍 Looks great! Nice work.

Fixes XRPLF#2525

intelliot · 2023-02-18T06:01:33Z

@drlongle if you feel this is ready to merge, you can put the Passed label on it

* upstream/develop: Rectify the import paths of boost/iterator: (XRPLF#4293) Allow port numbers be be specified with a colon: (XRPLF#4328) Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411) Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)

…ctionality * upstream/develop: Rectify the import paths of boost/iterator: (XRPLF#4293) Allow port numbers be be specified with a colon: (XRPLF#4328) Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411) Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)

…tpage * upstream/develop: Rectify the import paths of boost/iterator: (XRPLF#4293) Allow port numbers be be specified with a colon: (XRPLF#4328) Use <=> operator for base_uint, Issue, and Book: (XRPLF#4411) Reporting Mode: Do not attempt to acquire missing data from peer network (XRPLF#4458)

intelliot requested review from scottschurr and nbougalis February 7, 2023 17:04

intelliot assigned nbougalis and scottschurr Feb 8, 2023

nbougalis suggested changes Feb 8, 2023

View reviewed changes

nbougalis approved these changes Feb 12, 2023

View reviewed changes

scottschurr reviewed Feb 15, 2023

View reviewed changes

scottschurr approved these changes Feb 16, 2023

View reviewed changes

src/ripple/protocol/Book.h Outdated Show resolved Hide resolved

src/test/basics/base_uint_test.cpp Show resolved Hide resolved

intelliot unassigned nbougalis and scottschurr Feb 17, 2023

scottschurr approved these changes Feb 17, 2023

View reviewed changes

Use <=> operator for base_uint, Issue, and Book (XRPLF#4411)

f7daa6d

Fixes XRPLF#2525

drlongle force-pushed the retire-compare-func-for-base-uint branch from e873b55 to f7daa6d Compare February 17, 2023 17:04

drlongle added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 19, 2023

intelliot added this to the 1.10.1 milestone Feb 21, 2023

intelliot merged commit 84cde3c into XRPLF:develop Mar 15, 2023

intelliot changed the title ~~Replace compare() with the three-way comparison operator in base_uint, Issue and Book~~ Use <=> operator for base_uint, Issue, and Book: Jun 20, 2023

shortthefomo mentioned this pull request Oct 22, 2024

Build is failing can't figure out why. Xahau/xahaud#380

Open


		else if (relation > 0)
		if (relation == std::strong_ordering::greater)

		static_assert(std::is_copy_constructible<test96>::value, "");
		static_assert(std::is_copy_assignable<test96>::value, "");

Use <=> operator for base_uint, Issue, and Book: #4411

Use <=> operator for base_uint, Issue, and Book: #4411

Conversation

drlongle commented Feb 7, 2023 • edited Loading

High Level Overview of Change

Type of Change

drlongle commented Feb 7, 2023

drlongle commented Feb 8, 2023

nbougalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbougalis left a comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottschurr left a comment

Choose a reason for hiding this comment

scottschurr commented Feb 16, 2023

scottschurr left a comment

Choose a reason for hiding this comment

intelliot commented Feb 18, 2023

drlongle commented Feb 7, 2023 •

edited

Loading