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

[naoto.sato at oracle.com]

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
>>>>>>>
>>>>>
>>>
>>
>