Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revisit need for compare in base_uint.h #2525

Closed
JoeLoser opened this issue May 13, 2018 · 1 comment · Fixed by #4411
Closed

Revisit need for compare in base_uint.h #2525

JoeLoser opened this issue May 13, 2018 · 1 comment · Fixed by #4411
Assignees
@drlongle
Labels
Good First Issue Great issue for a new contributor Low Priority Reviewed Tech Debt Non-urgent improvements

Comments

@JoeLoser
Copy link
Contributor

@nbougalis brought up a good point about whether we actually need compare in base_uint.h after #2514.

This ticket should revisit this and see if it is truly needed. It seems that the underlying representation of base_uint should form a natural total ordering. So, ideally, these steps would form a natural transition of removing compare:

  1. Make the comparison operators in base_uint.h delegate to those of std::array which form a lexicographical comparison. You can just implement operator< and operator== as friends in the class definition and the other four outside as non-friends. Change the tests in base_uint_test.cpp to use the comparison operators directly instead of compare.
  2. Rewrite compare function in Issue.cpp to leverage these comparison operators directly in base_uint.h.
  3. Decide what to do about passing the original compare function object from base_uint.h around inside addRaw in TxMeta.cpp.
  4. Remove compare in base_uint.h.
@JoeLoser JoeLoser mentioned this issue May 13, 2018
Closed
@nbougalis
Copy link
Contributor

I like this. If you want to do this @JoeLoser that would be great.

@mDuo13 mDuo13 added the Tech Debt Non-urgent improvements label Aug 2, 2019
@carlhua carlhua added Low Priority Reviewed Good First Issue Great issue for a new contributor labels Sep 16, 2020
@drlongle drlongle self-assigned this Feb 1, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in Core Ledger Feb 1, 2023
@intelliot intelliot moved this from 🆕 New to 📋 Backlog in Core Ledger Feb 1, 2023
@drlongle drlongle mentioned this issue Feb 7, 2023
Merged
8 tasks
drlongle added a commit to drlongle/rippled that referenced this issue Feb 17, 2023
@drlongle
Use <=> operator for base_uint, Issue, and Book (XRPLF#4411)
f7daa6d 
Fixes XRPLF#2525
@intelliot intelliot moved this from 📋 Backlog to 🏗 In progress in Core Ledger Feb 18, 2023
intelliot pushed a commit that referenced this issue Mar 15, 2023
@drlongle
Use <=> operator for base_uint, Issue, and Book: (#4411)
84cde3c 
- Implement 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<=`, ...).
- Remove the function compare() because it is no longer needed.
- Maintain the same semantics as the existing code.
- Add some unit tests to gain further confidence.
- Fix #2525.
@intelliot intelliot moved this from 🏗 In progress to ✅ Merged in Core Ledger Apr 20, 2023
@manojsdoshi manojsdoshi moved this from ✅ Merged to Ready for the release branch in Core Ledger Apr 21, 2023
@intelliot intelliot moved this from Ready for the release branch to 🚢 Released in 1.11 in Core Ledger Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@drlongle drlongle

Labels
Good First Issue Great issue for a new contributor Low Priority Reviewed Tech Debt Non-urgent improvements
Projects
Core Ledger
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use <=> operator for base_uint, Issue, and Book: drlongle/rippled
5 participants
@nbougalis @JoeLoser @mDuo13 @carlhua @drlongle