RFR: JDK-8013900: More warnings compiling jaxp. (original) (raw)

Daniel Fuchs daniel.fuchs at oracle.com
Wed May 15 09:42:09 UTC 2013

Hi,

Please find below a revised version of the fix for JDK-8013900: More warnings compiling jaxp. <http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.01/>

Difference with the previous version [1] are in LocalVariableGen.java, plus 4 additional files: BranchInstruction.java CodeExceptionGen.java LineNumberGen.java Select.java

This new set of changes fixes an additional issue I discovered in LocalVariableGen.java - while running the JCK tests.

LocalVariableGen had a bug that was hidden by the fact that hashCode() was not compliant with equals(). Changing hashCode() to be compliant with equals() uncovered that issue.

The issue with LocalVariableGen is that the instance variables used by equals/hashCode are mutable. This is an issue because instances of LocalVariableGen are stored in an HashSet (targeters) held from instances of InstructionHandle.

Whenever the instruction handles in LocalVariableGen are changed, then the instance of LocalVariableGen was removed from the 'old' instruction handle targeters HashSet, and added to the 'new' instruction handle targeters HashSet - (see LocalVariableGen updateTargets(..), LocalVariableGen.setStart(...) & LocalVariableGen.setEnd(...).

The issue here was that the instance of LocalVariableGen was removed from the 'old' instruction handle targeters HashSet before being modified (which was correct) - but was also added to the 'new' instruction handle targeters HashSet before being modified - which was incorrect because at that time the hashCode() was still computed using the 'old' instruction handle, and therefore had its 'old' value. (this was done by BranchInstruction.notifyTarget(...))

The fix for this new issue is to split BranchInstruction.notifyTarget(...) in two methods: BranchInstruction.notifyTargetChanging(...) which is called before the instruction handle pointer is changed, and remove the LocalVariableGen from the old instruction handle targeters HashSet, and BranchInstruction.notifyTargetChanged(...), which is called after the instruction handle pointer is changed, and adds the LocalVariableGen to the new instruction handle targeters HashSet. In LocalVariableGen - whenever one of the instruction handles that take parts in the hashCode computation is changed, we unregister 'this' from all targeters HashSets in which it is registered, then modify the instruction handle pointer, then add back 'this' to all targeters HashSets in which it needs to be registered.

The 4 additional files in the webrev were also calling BranchInstruction.notifyTarget - and I modified them to call the two new methods instead of the old one - but without going through the trouble of unregistering 'this' from any known HashSet before modifictation - and adding it after, since those other classes do not have mutable hashCode().

Note: I also took this opportunity to change the method calling BranchInstruction.notifyTargetXxxx to be final, as I think it's desirable that they not be overridden, and to make the index in LocalVariableGen final - since it was never changed after the instance construction (and takes part in the HashCode computation).

best regards,

-- daniel

previous version: [1] <http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.00/>

On 5/13/13 4:36 PM, Daniel Fuchs wrote:

This is a fix for JDK-8013900: More warnings compiling jaxp.

<http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.00/> Although the title might suggest a trivial fix, it goes a bit beyond a simple warning fix because the root cause of those warnings is that some classes redefine equals without redefining hashCode() - and devising a hashCode() method that respects its contract with equals() is not always trivial. The proposed fix adds hashCode() methods where necessary, ensuring that: if (o1.equals(o2) == true), then (o1.hashCode() == o2.hashCode()) The fix also contains some cosmetic/sanity changes - like: 1. adding @Override wherever NetBeans complained they were missing - and 2. replacing StringBuffer with StringBuilder when possible. 3. In one instance, AbstractDateTimeDV.java I also had to reformat the whole file (due to its weird original formatting) - apologies for the noise it causes in the webrev. 4. I also removed a couple of private isEqual(obj1, obj2) methods replacing them by calls to Objects.equals(obj1, obj2) which did the same thing. 5. finally I refactored some of the existing equals (e.g. replacing try { (Sometype) other.... } catch (ClassCastException x) { return false; } with use of 'other instanceof Sometype'...) There are however a couple of more serious changes that could deserve to be reviewed in more details: a. The new implementation of hashCode() in AbstractDateTimeDV.DateTimeData, where I had to figure out a way to convert the date to UTC so that the hashCode() would match equals: AbstractDateTimeDV.java: lines 972-992 and b. in PrecisionDecimalDV.XPrecisionDecimal - where I had to invent a canonical string representation to devise a hashCode that would match equals: PrecisionDecimalDV.java: lines 147-237 For this last one - I have added a new test in jdk/test to check the implementation of the new canonical String representation for hashCode() - since the code of that is not trivial.

best regards, -- daniel

More information about the core-libs-dev mailing list