RFR: JDK-8013900: More warnings compiling jaxp.

Chris Hegarty chris.hegarty at oracle.com
Wed May 15 11:56:48 UTC 2013

I skimmed over the hashCode/equals parts of the changes, and they look 
fine to me.


On 15/05/2013 10:42, Daniel Fuchs wrote:
> 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