RFR [16/java.xml] 8246816: XMLGregorianCalendar.hashCode() produces far too many identical hashes

[Joe Wang]

Hi Naoto,

Why not :-). I kept it to go with equals(). Removing both now. Added a 
reference comparison as did in the Impl class.

http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_04/

Joe

On 8/7/20 10:50 AM, naoto.sato at oracle.com wrote:
> Hi Joe,
>
> If the new XMLGregorianCalendarImpl.hashCode() just calls 
> super.hashCode(), could it be removed entirely? Also 
> XMLGregorianCalendarImpl.equals() seems to do the same thing as its 
> parent. Could it also be removed?
>
> Naoto
>
> On 8/7/20 10:16 AM, Joe Wang wrote:
>> Naoto, Roger,
>>
>> Sorry have to come back with another iteration. It looks like the 
>> addition of getMillisecond() over the original implementation does 
>> change things.
>>
>> In particular, getMillisecond() returns FIELD_UNDEFINED when 
>> fractionalSecond == null (or not specified). But the normalization 
>> process will set it to zero after a mathematics adjustment. Now this 
>> will fail when comparing with a datetime that does not need to be 
>> normalized and thereby still returns FIELD_UNDEFINED (FIELD_UNDEFINED 
>> = Integer.MIN_VALUE). This situation is demonstrated in tests 3 at 
>> line 56 and 4 at line 58. 
>> <http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_03/test/jaxp/javax/xml/jaxp/unittest/datatype/HashCodeTest.java.html>
>>
>> The culprit is XMLGregorianCalendarImpl::normalizeToTimezone. In the 
>> original implementation, XMLGregorianCalendarImpl was calling the 
>> private normalizeToTimezone while XMLGregorianCalendar the public 
>> normalize(). The later will do the right thing to set the Millisecond 
>> back to UNDEFINED.
>>
>> In webrev_03, XMLGregorianCalendarImpl now calls super.hashCode() 
>> since the above was the only difference between the hashCode() impls 
>> of XMLGregorianCalendarImpl and XMLGregorianCalendar.
>>
>> webrev_03:
>> http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_03/
>>
>> Thanks,
>> Joe
>>
>>
>> On 8/6/20 1:51 PM, Joe Wang wrote:
>>> Thanks Naoto, Roger!  I'll prepare push based on version 01.
>>>
>>> Best,
>>> Joe
>>>
>>> On 8/6/20 1:29 PM, Roger Riggs wrote:
>>>> +1
>>>>
>>>> On 8/6/20 4:18 PM, naoto.sato at oracle.com wrote:
>>>>> Hi Joe, thank you for looking it into further.
>>>>>
>>>>> Yes, I agree the chances of collision is very rare, as 
>>>>> sub-millisecond difference in two objects. So fine with the 
>>>>> version 01.
>>>>>
>>>>> Naoto
>>>>>
>>>>> On 8/6/20 12:18 PM, Joe Wang wrote:
>>>>>> Thanks Naoto, Roger for the review!
>>>>>>
>>>>>> For Naoto's concern about the equals method using EonAndYear and 
>>>>>> FractionalSecond, I did cut corners using the all int getXXX 
>>>>>> method. The rational was: it fulfills the equals-hashcode 
>>>>>> contract just fine, is consistent with the existing 
>>>>>> implementation, and concise. This API was introduced since 1.5, 
>>>>>> but I have yet to see any usage of EonAndYear, and very rarely 
>>>>>> FractionalSecond. The chances collisions occur due to these 
>>>>>> differences are very low.
>>>>>>
>>>>>> But I understand your concern. To be precise or exact as 
>>>>>> equals(), we would need to call getFractionalSecond and 
>>>>>> EonAndYear. Here's what that would look like:
>>>>>> http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_02/
>>>>>>
>>>>>> To me, I like version 1 for the reasons above:
>>>>>> http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev_01/
>>>>>>
>>>>>> What would you think?
>>>>>>
>>>>>> Regards,
>>>>>> Joe
>>>>>>
>>>>>> On 8/5/20 6:13 PM, naoto.sato at oracle.com wrote:
>>>>>>> Hi Joe,
>>>>>>>
>>>>>>> For the consistency with equals(), just wondering if the 
>>>>>>> sub-second element should be obtained with getFractionalSecond() 
>>>>>>> instead of getMillisecond(), as the equals() subsequently calls 
>>>>>>> it in internalCompare() method. Also should it call 
>>>>>>> getEonAndYear() appropriately for the year?
>>>>>>>
>>>>>>> Naoto
>>>>>>>
>>>>>>> On 8/5/20 4:37 PM, Joe Wang wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> Please review a fix for the hashCode generation.
>>>>>>>>
>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8246816
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~joehw/jdk16/8246816/webrev/
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Joe
>>>>>>>>
>>>>>>
>>>>
>>>
>>