RFR: 8028185 - XMLFormatter.format emits incorrect year

[Stuart Marks]

On 11/19/13 9:40 AM, Mandy Chung wrote:
> On 11/19/13 2:23 AM, Daniel Fuchs wrote:
>> Hi,
>>
>> Please find below a webrev for:
>>
>> 8028185: XMLFormatter.format emits incorrect year
>> https://bugs.openjdk.java.net/browse/JDK-8028185
>>
>> The fix is trivial:
>> http://cr.openjdk.java.net/~dfuchs/webrev_8028185/webrev.00/
>
> Looks good.   Nit: the test can use StringBuilder which is generally in
> preference to StringBuffer unless synchronization is required.
>
> The kind of warning fix [1] causing this regression should probably have a
> regression test to go with it to verify the change.
>
> thanks
> Mandy
> [1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c1f129f62f36

Interesting and subtle. To save others the research, the changeset that 
introduced the regression removed a deprecation warning by replacing

     Date.getYear() + 1900

with

     Calendar.get(Calendar.YEAR) + 1900

This is incorrect and thus introduced the regression. The javadoc for 
Date.getYear() says to replace calls to it with:

     Calendar.get(Calendar.YEAR) - 1900

So, during warnings cleanup, a careful review of the code change plus the 
javadoc for the deprecated method being replaced would have revealed the error. 
Obviously, though, it's difficult always to be sufficiently careful in practice.

Alan said earlier,

> This one is a good reminder as to how fixing warnings can break things.

I think this is true. The warnings fixes aren't risk-free. For example, even 
generifying code can introduce ClassCastExceptions if the original code were to 
put values of different types into collections. This style has always been rare, 
but older code would do things like that.

Instead of writing regression tests for warnings fixes, though, I think I'd 
rather have good unit tests for a particular area. That way, general 
maintenance, as well as cleanups (like fixing warnings) are better protected 
against regressions.

s'marks