fix: Fix Number comparison operator by ximinez · Pull Request #7406 · XRPLF/rippled

ximinez · 2026-06-04T16:10:39Z

High Level Overview of Change

In operator<, if two Numbers have the same exponent and the same sign, then the final determination is done by comparing the unsigned mantissas directly. If they are both negative, and not equal it returns the wrong result.

Without the renames, comment changes, and test updates, this fix boils down to

        // If equal signs and exponents, compare mantissas.
        if (lneg)
            // If negative, the operator is reversed.
            return l.mantissa_ > r.mantissa_;

Context of Change

I went through all the Number comparisons, and as far as I can determine, all of the ones related to ledger data are guaranteed to have at least one parameter that is non-negative. That means there are no negative to negative comparisons, so we're safe to fix this directly.

Fortunately, the ledger just doesn't deal with a lot of negative numbers. Even with subtraction operations, the results don't go negative (or are checked against zero).

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

codecov · 2026-06-04T17:17:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.3%. Comparing base (949887f) to head (7944cbf).

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #7406   +/-   ##
=======================================
  Coverage     82.3%   82.3%           
=======================================
  Files         1011    1011           
  Lines        76913   76915    +2     
  Branches      8964    8964           
=======================================
+ Hits         63336   63339    +3     
+ Misses       13568   13567    -1     
  Partials         9       9

Files with missing lines	Coverage Δ
include/xrpl/basics/Number.h	`98.4% <100.0%> (+<0.1%)`	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

gregtatcam

Looks good to me. I left one suggestion to consider regarding the unit-tests readability.

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

This reverts commit a28e577.

- Test combinatorically with a large variety of values.

- Separate the int values from the non-int values.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

…ut the fix" This reverts commit c42a478bb48a578aec670fc1c9ea9dc78208f935.

ximinez · 2026-06-04T23:56:26Z

Rebased on top of the release/3.2.x branch to make merging into the release easier.

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

    friend constexpr bool
-    operator<(Number const& x, Number const& y) noexcept
+    operator<(Number const& l, Number const& r) noexcept
    {


vvysokikh1

While I think this PR looks good, I was just wondering if it worth switching to operator<=> here to let the compiler do the job. I made a quick pr (#7410) in case you're interested

ximinez · 2026-06-05T15:31:15Z

While I think this PR looks good, I was just wondering if it worth switching to operator<=> here to let the compiler do the job. I made a quick pr (#7410) in case you're interested

@vvysokikh1 Great suggestion and PR. Let's save it for 3.3 so it can get a more thorough looking at.

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

xrplf-ai-reviewer

No issues.

Review by Claude Sonnet 4.6 · Prompt: V15

ximinez changed the title ~~Comment out most Number comparisons~~ fix: Fix Number comparison operator Jun 4, 2026

ximinez marked this pull request as draft June 4, 2026 16:11

xrplf-ai-reviewer Bot approved these changes Jun 4, 2026

View reviewed changes

ximinez added this to the 3.2.0 milestone Jun 4, 2026

ximinez marked this pull request as ready for review June 4, 2026 17:32

xrplf-ai-reviewer Bot approved these changes Jun 4, 2026

View reviewed changes

ximinez requested a review from Copilot June 4, 2026 18:27

Copilot started reviewing on behalf of ximinez June 4, 2026 18:27 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

gregtatcam reviewed Jun 4, 2026

View reviewed changes

Comment thread src/test/basics/Number_test.cpp

gregtatcam approved these changes Jun 4, 2026

View reviewed changes

xrplf-ai-reviewer Bot approved these changes Jun 4, 2026

View reviewed changes

ximinez requested a review from Copilot June 4, 2026 23:27

Copilot started reviewing on behalf of ximinez June 4, 2026 23:27 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread src/test/basics/Number_test.cpp

xrplf-ai-reviewer Bot approved these changes Jun 4, 2026

View reviewed changes

ximinez changed the base branch from develop to release/3.2.x June 4, 2026 23:52

ximinez added 11 commits June 4, 2026 19:53

Comment out most Number comparisons

00d46c5

Revert "Comment out most Number comparisons"

89c38e6

This reverts commit a28e577.

Add test cases to testRelationals to show the problem

7ed0004

Fix the problem

52c6652

Improve readability

4ca1c6d

clang-format complains about the equality tests

7f2d18f

Make the Relational tests way more comprehensive

35521e1

- Test combinatorically with a large variety of values.

Clean up the relationals Number test for readability

763bba2

- Separate the int values from the non-int values.

Include vector

cdcae49

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Remove the fix: Use this commit to show that tests fail without the fix

8b6b075

Revert "Remove the fix: Use this commit to show that tests fail witho…

a489708

…ut the fix" This reverts commit c42a478bb48a578aec670fc1c9ea9dc78208f935.

ximinez force-pushed the ximinez/number-fix-comparison branch from 58cdf96 to a489708 Compare June 4, 2026 23:56

xrplf-ai-reviewer Bot approved these changes Jun 4, 2026

View reviewed changes

ximinez requested a review from Copilot June 5, 2026 00:00

Copilot started reviewing on behalf of ximinez June 5, 2026 00:00 View session

Copilot AI reviewed Jun 5, 2026

View reviewed changes

Comment thread include/xrpl/basics/Number.h

Comment on lines 410 to 412

friend constexpr bool

operator<(Number const& x, Number const& y) noexcept

operator<(Number const& l, Number const& r) noexcept

{

vvysokikh1 approved these changes Jun 5, 2026

View reviewed changes

ximinez added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jun 5, 2026

clang-tidy: if {}

e074cd4

xrplf-ai-reviewer Bot approved these changes Jun 5, 2026

View reviewed changes

ximinez changed the base branch from release/3.2.x to develop June 5, 2026 21:24

Merge branch 'develop' into ximinez/number-fix-comparison

7944cbf

xrplf-ai-reviewer Bot approved these changes Jun 5, 2026

View reviewed changes

Conversation

ximinez commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

API Impact

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gregtatcam left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

ximinez commented Jun 4, 2026

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

vvysokikh1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ximinez commented Jun 5, 2026

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ximinez commented Jun 4, 2026 •

edited

Loading

codecov Bot commented Jun 4, 2026 •

edited

Loading

vvysokikh1 left a comment •

edited

Loading