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

Change mulDiv() return from pair<bool, std::uint64_t> to optional<std::uint64_t> #3495

Closed
carlhua opened this issue Jul 8, 2020 · 1 comment · Fixed by #4243
Closed

Change mulDiv() return from pair<bool, std::uint64_t> to optional<std::uint64_t> #3495

carlhua opened this issue Jul 8, 2020 · 1 comment · Fixed by #4243
Assignees
@HowardHinnant
Labels
RIPD Export Exported from legacy JIRA issue tracking Tech Debt Non-urgent improvements

Comments

@carlhua
Copy link
Contributor

carlhua commented Jul 8, 2020

The current mulDiv interface is error prone, and it’s easy to ignore when overflow occurs. The suggestion is that we change this interface to use optional in place of pair<boool, value>.

This suggestion should decrease the likelihood of people ignoring overflow. It’s also very convenient to use optional::value_or() as a way to explicitly recover from overflow.

Exported from RIPD-1791
@carlhua carlhua added Tech Debt Non-urgent improvements RIPD Export Exported from legacy JIRA issue tracking labels Jul 8, 2020
@ckeshava ckeshava mentioned this issue Jul 19, 2022
Merged
ckeshava pushed a commit to ckeshava/rippled that referenced this issue Jul 19, 2022
execute git-clang format, fixes XRPLF#3495
f3f0423
ckeshava pushed a commit to ckeshava/rippled that referenced this issue Jul 21, 2022
fixes XRPLF#3495 - change return type of mulDiv into std::optional
ed9f30a
@intelliot
Copy link
Collaborator

May be fixed by #4243

@intelliot intelliot linked a pull request Mar 14, 2023 that will close this issue
Change the return type of mulDiv to std::optional #4243
Merged
HowardHinnant pushed a commit to HowardHinnant/rippled that referenced this issue May 22, 2023
@HowardHinnant
fixes XRPLF#3495 - change return type of mulDiv into std::optional
d2352fe
ckeshava pushed a commit to ckeshava/rippled that referenced this issue Jun 5, 2023
@ckeshava
fixes XRPLF#3495 - change return type of mulDiv into std::optional
6946cb6 
This commit will include limits header file for satisfying gcc's numeric_limits incomplete_type requirement. Interestingly, the build system on MacOS does not throw an error despite the lack of #include <limits.h> preprocessing directive.

Certain unused imports are removed from mulDiv implementation file
ckeshava pushed a commit to ckeshava/rippled that referenced this issue Jul 5, 2023
@thejohnfreeman @ckeshava
refactor: change the return type of mulDiv to std::optional (XRPLF#4243)
03e2404 
- Previously, mulDiv had `std::pair<bool, uint64_t>` as the output type.
  - This is an error-prone interface as it is easy to ignore when
    overflow occurs.
- Using a return type of `std::optional` should decrease the likelihood
  of ignoring overflow.
  - It also allows for the use of optional::value_or() as a way to
    explicitly recover from overflow.
- Include limits.h header file preprocessing directive in order to
  satisfy gcc's numeric_limits incomplete_type requirement.

Fix XRPLF#3495

---------

Co-authored-by: John Freeman <[email protected]>
intelliot pushed a commit that referenced this issue Jul 6, 2023
@ckeshava @thejohnfreeman
refactor: change the return type of mulDiv to std::optional (#4243)
c6fee28 
- Previously, mulDiv had `std::pair<bool, uint64_t>` as the output type.
  - This is an error-prone interface as it is easy to ignore when
    overflow occurs.
- Using a return type of `std::optional` should decrease the likelihood
  of ignoring overflow.
  - It also allows for the use of optional::value_or() as a way to
    explicitly recover from overflow.
- Include limits.h header file preprocessing directive in order to
  satisfy gcc's numeric_limits incomplete_type requirement.

Fix #3495

---------

Co-authored-by: John Freeman <[email protected]>
ckeshava added a commit to ckeshava/rippled that referenced this issue Jul 10, 2023
@ckeshava @thejohnfreeman
refactor: change the return type of mulDiv to std::optional (XRPLF#4243)
95d67eb 
- Previously, mulDiv had `std::pair<bool, uint64_t>` as the output type.
  - This is an error-prone interface as it is easy to ignore when
    overflow occurs.
- Using a return type of `std::optional` should decrease the likelihood
  of ignoring overflow.
  - It also allows for the use of optional::value_or() as a way to
    explicitly recover from overflow.
- Include limits.h header file preprocessing directive in order to
  satisfy gcc's numeric_limits incomplete_type requirement.

Fix XRPLF#3495

---------

Co-authored-by: John Freeman <[email protected]>
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 22, 2023
@ckeshava @thejohnfreeman
refactor: change the return type of mulDiv to std::optional (XRPLF#4243)
c013763 
- Previously, mulDiv had `std::pair<bool, uint64_t>` as the output type.
  - This is an error-prone interface as it is easy to ignore when
    overflow occurs.
- Using a return type of `std::optional` should decrease the likelihood
  of ignoring overflow.
  - It also allows for the use of optional::value_or() as a way to
    explicitly recover from overflow.
- Include limits.h header file preprocessing directive in order to
  satisfy gcc's numeric_limits incomplete_type requirement.

Fix XRPLF#3495

---------

Co-authored-by: John Freeman <[email protected]>
ckeshava added a commit to ckeshava/rippled that referenced this issue Sep 25, 2023
@ckeshava @thejohnfreeman
refactor: change the return type of mulDiv to std::optional (XRPLF#4243)
926e488 
- Previously, mulDiv had `std::pair<bool, uint64_t>` as the output type.
  - This is an error-prone interface as it is easy to ignore when
    overflow occurs.
- Using a return type of `std::optional` should decrease the likelihood
  of ignoring overflow.
  - It also allows for the use of optional::value_or() as a way to
    explicitly recover from overflow.
- Include limits.h header file preprocessing directive in order to
  satisfy gcc's numeric_limits incomplete_type requirement.

Fix XRPLF#3495

---------

Co-authored-by: John Freeman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@HowardHinnant HowardHinnant

Labels
RIPD Export Exported from legacy JIRA issue tracking Tech Debt Non-urgent improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Change the return type of mulDiv to std::optional ckeshava/rippled
3 participants
@intelliot @HowardHinnant @carlhua