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.

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

8262744: Formatter '%g' conversion uses wrong format for BigDecimal rounding up to limits #3363

Closed
Closed
Changes from 1 commit
Commits
File filter
Filter file types
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -3821,12 +3821,10 @@ private void print(StringBuilder sb, BigDecimal value, Locale l,
else if (precision == 0)
prec = 1;

BigDecimal tenToTheNegFour = BigDecimal.valueOf(1, 4);
BigDecimal tenToThePrec = BigDecimal.valueOf(1, -prec);
value = value.round(new MathContext(prec));
This conversation was marked as resolved by igraves

This comment has been minimized.

@RogerRiggs

RogerRiggs Apr 16, 2021
Contributor

While you are in the area, how about inlining the creation of the tenToTheNegFour and tenToThePrec values.
They are used only once and may not be used at all. They don't appear to be needed except for the comparisons.

This comment has been minimized.

@igraves

igraves Apr 16, 2021
Author Member

Makes sense to me.

if ((value.equals(BigDecimal.ZERO))
|| ((value.compareTo(tenToTheNegFour) != -1)
&& (value.compareTo(tenToThePrec) == -1))) {
|| ((value.compareTo(BigDecimal.valueOf(1, 4)) != -1)
&& (value.compareTo(BigDecimal.valueOf(1, -prec)) == -1))) {

This comment has been minimized.

@stuart-marks

stuart-marks Apr 21, 2021
Member

Note that compareTo in general specifies a negative, zero, or positive return value, but BigDecimal and BigInteger specify a return value of -1, 0, and 1. So the code here that compares against -1 is strictly correct. However, the BigDecimal/BigInteger.compareTo docs say "The suggested idiom..." is a relative comparison against zero.

Indeed, the BigDecimal::compareTo method does always seem to return -1, 0, or 1 so this code is not incorrect. Well, maybe. I checked quickly and the BigDecimal comparison logic is fairly intricate (and also runs through BigInteger) so I might have missed something. Also, BigDecimal is subclassable, so an override of compareTo might return something other than -1, 0, or 1, even though strictly speaking this would violate the BigDecimal spec.

I'm wondering if there should be a followup bug that changes these tests to >= 0 and < 0.


int e = - value.scale()
+ (value.unscaledValue().toString().length() - 1);
ProTip! Use n and p to navigate between commits in a pull request.