Code review request for #6469160, #7088271: (fmt) format("%.1g", 0.0) throws ArrayOutOfBoundsException

[David Holmes]

Hi Brandon,

This point fix still bothered me so I dug a bit deeper into what the 
code does (as complex and convoluted as it is). I wanted to compare the 
cases of 0.0 and 1.0 to see why they differed and it seemed to me that 
the difference was introduced way back when the FormattedFloatingDecimal 
is created. If we detect a zero we initialize explicitly as:

     decExponent = 0;
     digits = zero;
     nDigits = 1;

whereas with 1.0 we'd have a decExponent of 1. Later in the code (at the 
site of your original change) we have:

                } else {
                    form = Form.DECIMAL_FLOAT;
                    precision = precision - exp;
                }

which will leave precision unchanged at 1 for 0.0 ( 1 minus 0) but will 
drop it to 0 for 1.0 ( 1 minus 1).  Your original fix hardwired changing 
the precision to 0 if dealing with 0. It seemed to me that if we go back 
to that constructor and instead set decExponent == 1 for the zero case, 
then we will later get the desired precision==0 and avoid the 
ArrayOutOfBoundsException. And we do, but not for that reason. By 
setting decExponent to 1 we enter the "print digit.digits" branch rather 
than the "print 0.0*" branch - and it works (though not exhaustively 
tested).

Your updated patch tackles the "print 0.0* digits" path. We skip the 
first if block as exp==0. We would then hit:

              int t = Math.min(digits.length, precision + exp);
              if (t > 0) {

Your original patch will cause t==0 so we skip the if block. Your new 
patch guards the Math.min and the if block with an additional:

  if (precision != 1 || digits[0] != '0' || digits.length != 1) {

so we again skip the t>0 if block.

I'm left wondering of any of these three "fixes" are actually 'the right 
fix", or whether it really matters. Presuming a second reviewer is 
needed anyway perhaps we should just wait to see what they have to offer.

Apologies that this has (again) become far more time consuming than 
anticipated. I wish a domain expert would step in on this (assuming one 
still exists).

Thanks,
David

On 27/01/2012 11:50 AM, Brandon Passanisi wrote:
> Hi David. I have revised my fix for this CR. Here's a description of the
> most recent changes:
>
> 1. The code fix has been moved slightly from it's original position
> into a section within FormattedFloatingDecimal.java which handles
> 0.0* numbers.
>
> 2. The code fix no longer changes the precision value.
>
> 3. The code fix checks to see if the precision is 1 and the number
> is a one-digit 0. If not, the normal code which adds the decimal
> and remaining digits is run. If so, this block of code is not run.
>
> In addition, I have re-run the Basic-X set of Formatter tests to make
> sure there aren't any regressions.
>
> Webrev: http://cr.openjdk.java.net/~bpassani/6469160/1/webrev/
> <http://cr.openjdk.java.net/%7Ebpassani/6469160/1/webrev/>
>
>
> Thanks.
>
> On 1/24/2012 11:58 PM, David Holmes wrote:
>> 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.
>>>
>