Change the return type of mulDiv to std::optional #4243

ckeshava · 2022-07-19T20:48:59Z

Previously, mulDiv had std::pair<bool, uint64_t> as the output type.
This PR changes the return type to std::optional
The existing unit test cases pass with this change.
This PR attempts to fix Issue 3495 (Change mulDiv() return from pair<bool, std::uint64_t> to optional<std::uint64_t> #3495)

ckeshava · 2022-07-19T20:56:35Z

I had to use std::numeric_limits<std::uint64_t>::max() in a few places to return a default value (in case of an overflow). Do I need to store this value in a variable for better readability? Or is it okay to use this verbose expression?

src/ripple/basics/mulDiv.h

src/ripple/basics/impl/mulDiv.cpp

src/ripple/app/misc/impl/TxQ.cpp

src/ripple/app/misc/TxQ.h

src/ripple/app/tx/impl/DeleteAccount.cpp

src/ripple/ledger/ReadView.h

src/ripple/app/misc/TxQ.h

src/ripple/app/misc/impl/TxQ.cpp

src/ripple/app/tx/impl/DeleteAccount.cpp

greg7mdp · 2022-07-20T19:14:22Z

Nice, thanks @chennakeshava1998 , but I don't think it is necessary to leave the old code commented out.

ckeshava · 2022-07-20T20:49:29Z

@greg7mdp Sorry my bad. I forgot to remove it. Completed now 👍

greg7mdp

Almost there!

src/ripple/app/misc/TxQ.h

ckeshava · 2022-07-21T19:00:29Z

Almost there!

Done :)
I have also removed extra import statements and refactored one more if-else condition into a value_or statement.

Do I need to clean up the commits? (I can do a force push with a single commit)

greg7mdp · 2022-07-21T19:11:09Z

Nice! Looks very clean now.

Do I need to clean up the commits? (I can do a force push with a single commit)

Yes, I think it would be better.

Make sure to run the unittests again with the latest version, if you haven't already.

ckeshava · 2022-07-21T20:53:00Z

I have executed the unit tests, I didn't observe any failures. 👍

greg7mdp

Looks great!

greg7mdp · 2022-07-21T21:05:13Z

I approved the PR, but I don't have write access to the repo, so it doesn't count.

ckeshava · 2022-07-21T21:53:19Z

Okat, thanks for helping me out greg!

intelliot · 2023-03-14T15:07:01Z

@ckeshava could you merge develop into your branch, please?

ckeshava · 2023-03-14T16:12:24Z

Hello @intelliot ,
Sure, I'd love to merge it and clean up the commit. Is this a priority issue? I completed this code change as a part of my summer internship last year.

I do not have C++ toolchain required to install the dependencies and unit-test this change. Can I do this after 2-3 weeks?

intelliot · 2023-04-03T05:43:02Z

No hurry - thanks!

HowardHinnant · 2023-04-04T18:35:24Z

@ckeshava Could you rebase this against the current develop? I tried to do so and got multiple merge conflicts. And I don't want to try to guess the correct conflict resolutions.

I need it rebased because we've recently moved to conan for a build tool, and your PR won't build with Conan without a rebase.

ckeshava · 2023-04-04T19:00:24Z

@HowardHinnant Will do 👍

HowardHinnant

Needs a rebase to develop (has merge conflicts).

ckeshava · 2023-04-20T23:39:51Z

@HowardHinnant I have merged the latest develop into this feature branch.

intelliot · 2023-04-21T16:55:14Z

you can download the clang-format patch from https://github.com/XRPLF/rippled/actions/runs/4759912341?pr=4243 (under Artifacts) and apply it with git apply <patch file name>

ckeshava · 2023-04-21T17:08:34Z

thanks a lot elliot, I was searching for exactly that :))

ckeshava · 2023-04-25T22:57:57Z

Hello,
I have completed the code changes for this PR. @HowardHinnant @greg7mdp

HowardHinnant · 2023-05-30T18:40:38Z

I have requested changes to the git merge which place these commits at the top.

ckeshava · 2023-05-30T19:26:34Z

I'll work on your suggestions, sorry I forgot about it.

ckeshava · 2023-06-05T23:30:51Z

Hello @HowardHinnant ,
Thank you so much for your detailed help with reorganizing the git commits, it was really helpful.
Sorry for the delay in updating this PR, I got occupied with another PR.

Is there a better alternative than force-pushing the changes? After rebasing, the remote and local branches were so different that I couldn't think of a better alternative.

HowardHinnant · 2023-06-08T02:26:05Z

Hello @HowardHinnant , Thank you so much for your detailed help with reorganizing the git commits, it was really helpful. Sorry for the delay in updating this PR, I got occupied with another PR.

Is there a better alternative than force-pushing the changes? After rebasing, the remote and local branches were so different that I couldn't think of a better alternative.

I think force pushing is the only option here. Thanks for this work. It is on my to-do list to review.

thejohnfreeman · 2023-06-08T14:51:43Z

src/ripple/app/misc/NetworkOPs.cpp

@@ -2056,7 +2056,7 @@ NetworkOPsImp::pubServer()
                    f.em->openLedgerFeeLevel,
                    f.loadBaseServer,
                    f.em->referenceFeeLevel)
-                    .second);


Are cases like this guaranteed to have a value? Is that why the old code didn't check for success? If so, then I think the new code should just use operator*.

Cases like this are not guaranteed to have a value. When the old behavior overflowed, it passed back an overflow flag and the value ripple::muldiv_max. The client level code (in this example) didn't check the overflow flag and just took the value. In this rewrite, the client code is forced to be explicit that it is using ripple::muldiv_max on overflow.

In any case the behavior is unchanged.

src/ripple/basics/mulDiv.h

thejohnfreeman · 2023-06-14T23:32:11Z

CI caught a format issue. Instructions on how to fix it are in the job log.

ckeshava · 2023-06-14T23:46:35Z

okay, I've fixed that. let me know if I need to squash or clean-up the commit log

thejohnfreeman · 2023-06-15T03:29:32Z

It will be squashed when it is merged. Now that there are at least two approvals and all checks pass, you can add the "Passed" label if you think this PR is ready to merge.

ckeshava · 2023-06-15T17:42:06Z

It will be squashed when it is merged. Now that there are at least two approvals and all checks pass, you can add the "Passed" label if you think this PR is ready to merge.

Okay, done. thanks

intelliot · 2023-07-05T18:57:23Z

Suggested commit message:

refactor: change the return type of mulDiv to std::optional (#4243)

- 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]>

- 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 · 2023-07-06T03:11:03Z

For future reference: there is no need to force push. Instead, just confirm (or write a comment with) the desired commit message. A new commit message is set when the PR is Squash and merged.

Also, for future PRs, please sign your commits.

ckeshava · 2023-07-06T15:44:42Z

okay, I'll keep that in mind 👍

thejohnfreeman · 2023-07-10T14:28:05Z

Following up on an old question: @HowardHinnant @ckeshava please never force-push in a PR. If you end up in a situation where you don't want to bother resolving conflicts, and you just want to keep your changes, then use this:

git merge upstream/develop --strategy ours

This creates a merge commit with two parents, but just takes your changes wherever there is a conflict.

You should not rebase either. That is what creates the conditions (lost history) that encourages force-push.

ckeshava · 2023-07-10T17:11:17Z

Okay, thanks for the tip, I'll keep it in mind.

- 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 force-pushed the changeMulDiv branch from 1095f74 to 67d77ce Compare July 19, 2022 21:40

greg7mdp reviewed Jul 19, 2022

View reviewed changes

greg7mdp suggested changes Jul 19, 2022

View reviewed changes

src/ripple/app/misc/TxQ.h Outdated Show resolved Hide resolved

src/ripple/app/misc/impl/TxQ.cpp Outdated Show resolved Hide resolved

src/ripple/app/tx/impl/DeleteAccount.cpp Outdated Show resolved Hide resolved

greg7mdp suggested changes Jul 20, 2022

View reviewed changes

src/ripple/app/misc/TxQ.h Outdated Show resolved Hide resolved

ckeshava force-pushed the changeMulDiv branch 2 times, most recently from 3c3c6df to ed9f30a Compare July 21, 2022 20:33

greg7mdp approved these changes Jul 21, 2022

View reviewed changes

intelliot assigned ckeshava Mar 14, 2023

intelliot mentioned this pull request Mar 14, 2023

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

Closed

intelliot linked an issue Mar 14, 2023 that may be closed by this pull request

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

Closed

intelliot requested a review from HowardHinnant April 3, 2023 05:43

HowardHinnant suggested changes Apr 20, 2023

View reviewed changes

ckeshava force-pushed the changeMulDiv branch from b0cda2c to f579c1a Compare April 24, 2023 21:43

intelliot requested a review from drlongle April 26, 2023 05:36

ckeshava force-pushed the changeMulDiv branch from f579c1a to 6946cb6 Compare June 5, 2023 23:28

thejohnfreeman reviewed Jun 8, 2023

View reviewed changes

HowardHinnant approved these changes Jun 12, 2023

View reviewed changes

thejohnfreeman requested changes Jun 14, 2023

View reviewed changes

src/ripple/basics/mulDiv.h Outdated Show resolved Hide resolved

thejohnfreeman approved these changes Jun 14, 2023

View reviewed changes

ckeshava force-pushed the changeMulDiv branch from fa4a5ca to 2122cef Compare June 14, 2023 23:46

ckeshava self-assigned this Jun 15, 2023

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

intelliot removed the request for review from drlongle July 4, 2023 00:17

ckeshava force-pushed the changeMulDiv branch from 733f670 to 03e2404 Compare July 5, 2023 22:51

intelliot merged commit c6fee28 into XRPLF:develop Jul 6, 2023

ckeshava deleted the changeMulDiv branch July 6, 2023 15:44

Change the return type of mulDiv to std::optional #4243

Change the return type of mulDiv to std::optional #4243

Conversation

ckeshava commented Jul 19, 2022

ckeshava commented Jul 19, 2022

greg7mdp commented Jul 20, 2022

ckeshava commented Jul 20, 2022

greg7mdp left a comment

Choose a reason for hiding this comment

ckeshava commented Jul 21, 2022

greg7mdp commented Jul 21, 2022 • edited Loading

ckeshava commented Jul 21, 2022

greg7mdp left a comment

Choose a reason for hiding this comment

greg7mdp commented Jul 21, 2022

ckeshava commented Jul 21, 2022

intelliot commented Mar 14, 2023

ckeshava commented Mar 14, 2023

intelliot commented Apr 3, 2023

HowardHinnant commented Apr 4, 2023

ckeshava commented Apr 4, 2023

HowardHinnant left a comment

Choose a reason for hiding this comment

ckeshava commented Apr 20, 2023

intelliot commented Apr 21, 2023 • edited Loading

ckeshava commented Apr 21, 2023

ckeshava commented Apr 25, 2023

HowardHinnant commented May 30, 2023

ckeshava commented May 30, 2023

ckeshava commented Jun 5, 2023

HowardHinnant commented Jun 8, 2023

thejohnfreeman Jun 8, 2023

Choose a reason for hiding this comment

HowardHinnant Jun 12, 2023

Choose a reason for hiding this comment

thejohnfreeman commented Jun 14, 2023

ckeshava commented Jun 14, 2023

thejohnfreeman commented Jun 15, 2023

ckeshava commented Jun 15, 2023

intelliot commented Jul 5, 2023

intelliot commented Jul 6, 2023

ckeshava commented Jul 6, 2023

thejohnfreeman commented Jul 10, 2023

ckeshava commented Jul 10, 2023

greg7mdp commented Jul 21, 2022 •

edited

Loading

intelliot commented Apr 21, 2023 •

edited

Loading