I skimmed over the hashCode/equals parts of the changes, and they look
fine to me.
-Chris.
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
