rippled
rippled copied to clipboard
XRPLF
Change the return type of mulDiv to std::optional
- 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 (https://github.com/XRPLF/rippled/issues/3495)
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?
Nice, thanks @chennakeshava1998 , but I don't think it is necessary to leave the old code commented out.
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)
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.
I approved the PR, but I don't have write access to the repo, so it doesn't count.
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?
@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.
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>
Hello, I have completed the code changes for this PR. @HowardHinnant @greg7mdp
I have requested changes to the git merge which place these commits at the top.
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.
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.
okay, I've fixed that. let me know if I need to squash or clean-up the commit log
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.
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
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]>
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.
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.