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.s...@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.s...@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.s...@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
