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

[Joe Wang]

Thanks Roger!

Simpler indeed, one less place for mistakes ;-)

Removing the override did not change the javadoc for the public API.

Joe

On 8/7/20 12:44 PM, Roger Riggs wrote:
> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>