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
