RFR: JDK-8013900: More warnings compiling jaxp.

[huizhe wang]

Excellent work, Daniel!

The only minor problem for me is that I can't apply the patch directly 
to jaxp standalone since it uses JDK7 feature. But then we probably 
don't need to do it after all.

Thanks,
Joe

On 5/15/2013 2:42 AM, 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
>