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 all commits
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,11 +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);
@@ -0,0 +1,62 @@
/*
* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/**
* @test
* @bug 8262744
* @summary BigDecimal does not always display formatting correctly because
* rounding is done after formatting check. Fix moves rounding to before the
* range-based formatting check for %g formatting flag.
* @run testng BigDecimalRounding
*/

import org.testng.annotations.Test;

import java.math.BigDecimal;

import static org.testng.Assert.*;

@Test
public class BigDecimalRounding {

public static void testBigDecimalRounding() {
var res1 = String.format("%g", 0.00009999999999999995);
var res2 = String.format("%g", 0.00009999999f);
This conversation was marked as resolved by igraves

This comment has been minimized.

@jddarcy

jddarcy Apr 6, 2021
Member

To avoid possible vagaries in decimal -> binary conversion of double values, I suggest also testing against a BigDecimal directly rounded to the precision in question.

This comment has been minimized.

@igraves

igraves Apr 7, 2021
Author Member

Good thought, thanks.

var res3 = String.format("%g", new BigDecimal(0.0001));
var res4 = String.format("%g", new BigDecimal("0.00009999999999999999995"));

assertEquals(res1, res2);
assertEquals(res2, res3);
assertEquals(res3, res4);

var res5 = String.format("%.9g", 999999.999999432168754e+3);
var res6 = String.format("%.9g", 999999999.999432168754f);
var res7 = String.format("%.9g", new BigDecimal("999999.999999432168754e+3")); // !!
var res8 = String.format("%.9g", new BigDecimal("1000000000")); // !!

assertEquals(res5, res6);
assertEquals(res6, res7);
assertEquals(res7, res8);

}
}
ProTip! Use n and p to navigate between commits in a pull request.