Code review request for #6469160, #7088271

[Brandon Passanisi]

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:
> Hi Brandon,
>
> On 21/01/2012 4:19 AM, Brandon Passanisi wrote:
>> Resending again...
>>
>> Hello core-libs. I was wondering of somebody could be please review the
>> following fix for #6469160 and #7088271. The changes in the webrev fix
>> both bugs. Information is below:
>>
>> Webrev URL: http://cr.openjdk.java.net/~bpassani/6469160_7088271/1/
>> <http://cr.openjdk.java.net/%7Ebpassani/6469160_7088271/1/>
>> Bug #6469160: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6469160
>> Bug #7088271: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7088271
>
> Seems to me that 7088271 should be closed as a duplicate of 6469160.
>

Ok, I'll go ahead and close 7088271 as a duplicate.


>> Both bugs uncover the current behavior where using a 0 or 1 precision
>> value with a float zero causes an ArrayIndexOutOfBoundsException. The
>> current code in FormattedFloatingDecimal.java interprets the float zero
>> as "0.0" in the case where precision is 0 or 1 and returns the length of
>> it's characters as 3. Later in Formatter.addZeros(), the character array
>> "0.0" is passed in, but a new array of only 1 character is allocated.
>> When an System.arraycopy() is performed, the
>> ArrayIndexOutOfBoundsException occurs. In fact, when run with "-esa" an
>> AssertionError occurs at "assert (outPrec <= prec);" on line 3393 of
>> Formatter.java. The fix is for FormatedFloatingDecimal.java to interpret
>> the float zero as a single "0" because of the precision being set to 0
>> or 1.
>
> This isn't an area I'm familiar with so please excuse me if I'm 
> missing something obvious. I'm confused about the fix in regards to 
> the two cases reported in the CRs. In one case we have:
>
> String.format("%3.0g", 0.0);
>
> where the precision is 0. But in your fix you only special case the 
> situation where precision is 1:
>
> +           if (digits.length == 1 && digits[0] == '0'
> + && precision == 1) {
> +               // When the number is zero and precision is 1, set the
> +               // precision to 0 so that a decimal point and digits
> +               // are not added by code later in this method.
> +               precision--;
> +            } else {
>
> so I don't understand how this fixes %3.0g ?
>
> 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"

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.

-- 
Oracle <http://www.oracle.com>
Brandon Passanisi | Principle Member of Technical Staff


Green Oracle <http://www.oracle.com/commitment> Oracle is committed to 
developing practices and products that help protect the environment