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

[Roger Riggs]

Hi,

Simpler is better.

Does removing the override change the javadoc?  That might be considered 
a spec change.

Otherwise, looks good.

Roger


On 8/7/20 3:13 PM, Joe Wang wrote:
> 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
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>