RFR: JDK-8013900: More warnings compiling jaxp.

[Daniel Fuchs]

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