-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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.
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
8314169: Combine related RoundingMode logic in j.text.DigitList #15252
8314169: Combine related RoundingMode logic in j.text.DigitList #15252
Conversation
👋 Welcome back jlu! A progress list of the required criteria for merging this PR into |
@justin-curtis-lu The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
* Return true if there exists a non-zero digit in the digit list | ||
* from the given index until the end. | ||
*/ | ||
private boolean non0AfterIndex(int index) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer spelling out 0
to Zero
.
* This method is reserved for Long and BigInteger representations. | ||
* @param maximumDigits The maximum number of digits to be shown. | ||
* | ||
* Upon return, count will be less than or equal to maximumDigits. | ||
*/ | ||
private void round(int maximumDigits) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the method is only used for Long
and BigInteger
, probably use the specific name instead of explaining it in the description and overloading would be more clear to me.
if (non0AfterIndex(maximumDigits)) { | ||
return (isNegative && roundingMode == RoundingMode.FLOOR) | ||
|| (!isNegative && roundingMode == RoundingMode.CEILING); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
roundingMode
is checked against FLOOR
/CEILING
twice. I don't see the need of bundling them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. Addressed this, and your other comments that I did not explicitly respond to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd think this separate one-liner for each case would be simpler and more readable:
case CEILING:
return nonZeroAfterIndex(maximumDigits) && !isNegative;
case FLOOR:
return nonZeroAfterIndex(maximumDigits) && isNegative;
// Digit at rounding position is a '5'. Tie cases. | ||
if (maximumDigits != (count - 1)) { | ||
// There are remaining digits. Above tie => must round up | ||
case HALF_EVEN: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching (copy paste error)
if (alreadyRounded) { | ||
return false; | ||
// When exact, consider specific contract logic | ||
} else if (valueExactAsDecimal) { | ||
return (roundingMode == RoundingMode.HALF_UP) || | ||
(roundingMode == RoundingMode.HALF_EVEN | ||
&& (maximumDigits > 0) && (digits[maximumDigits - 1] % 2 != 0)); | ||
// Not already rounded, and not exact, round up | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this logic is equivalent to the previous one? Previously, HALF_UP/DOWN
checks valueExactAsDecimal
before alreadyRounded
, but the new one checks alreadyRounded
first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since alreadyRounded
and valueExactAsDecimal
are never both true
and when either of them are true
, it warrants a return without considering other logic.
However, I have adjusted the code so that this is more apparent (and appears more similar to the original HALF_DOWN/UP
, which was written more concisely).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks for confirming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@justin-curtis-lu This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 37 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 96778dd.
Your commit was automatically rebased without conflicts. |
@justin-curtis-lu Pushed as commit 96778dd. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this PR which is a broad clean up of the DigitList class (used by Format classes in j.text).
This PR is intended to be a portion of a bigger change (split up to make reviewing easier).
The main change simplifying related Rounding Mode logic in
shouldRoundUp()
- (CEILING/FLOOR, HALF_UP/DOWN/EVEN)Other changes include
roundInt(int)
- For use by Integer representations of DigitListnonZeroAfterIndex(int)
- To reduce code duplicationProgress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15252/head:pull/15252
$ git checkout pull/15252
Update a local copy of the PR:
$ git checkout pull/15252
$ git pull https://git.openjdk.org/jdk.git pull/15252/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15252
View PR using the GUI difftool:
$ git pr show -t 15252
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15252.diff
Webrev
Link to Webrev Comment