Code review request for #6469160, #7088271 (original) (raw)

David Holmes david.holmes at oracle.com
Wed Jan 25 07:58:27 UTC 2012

Hi Brandon,

On 25/01/2012 5:03 PM, Brandon Passanisi wrote:

Hi David. Thank you for your review comments. My answers to your questions are below:

On 1/23/2012 9:56 PM, David Holmes wrote: More generally it is not clear to me that putting in this special case is the right way to fix this. Though I admit I don't really understand what the specification requires when you give a precision of 0 with a 'g' conversion:

"If the conversion is 'g' or 'G', then the precision is the total number of digits in the resulting magnitude after rounding." So we asked for zero digits? What should that mean? The Formatter javadoc, within the "Float and Double" section, describes the following regarding a value of 0 for precision and the 'g'/'G' conversion [1]: "If the precision is 0, then it is taken to be 1"

Ah! Thanks. I hadn't seen that (wish they wouldn't split up the spec for this across different sections!).

Okay that explains the 0/1 situation.

But that makes me question the "rightness" of the fix even more. We took steps to change a precision of 0 to 1, but then the fix changes it back to 0 because otherwise something else breaks. Seems to me that the "something else" that handles the precision of 1 incorrectly is the code that really needs to be fixed. Further it suggest that there may be assumptions in later code that precision is in fact never 0.

David

The following code block within Formatter.java near line 3278 is run to do this:

if (precision == -1) prec = 6; else if (precision == 0) prec = 1; And the precision value "prec" is given to FormattedFloatingDecimal. Therefore, when this particular error condition is seen and the proposed code fix is reached in FormattedFloatingDecimal.java, the precision will be 1. So, the proposed code fix ends up supporting a precision value of 0 and 1.

[1]: http://docs.oracle.com/javase/7/docs/api/java/util/Formatter.html Thanks.

David ----- Since java has been throwing exceptions in these cases, I consulted with the output of C's printf to make sure that the outputted strings are the same. I updated the Formatter's Basic-X template of tests with a little over 20 test format strings that were causing exceptions without the change and the output of each is compared with the output from C's printf with the same format string. And, I ran all of the Basic-X tests to make sure there weren't any regressions in behavior.

Thanks.

More information about the core-libs-dev mailing list