Looks good, thanks for the update.
Roger
On 9/5/18 5:10 PM, naoto.s...@oracle.com wrote:
Hi Roger,
I updated the fix to share the zone only if the sharedZone is true.
Here is the updated webrev:
http://cr.openjdk.java.net/~naoto/8210142/webrev.01/
Naoto
On 9/5/18 7:07 AM, Roger Riggs wrote:
Hi Naoto,
That sounds like a test issue. I would not expect a cloned Calendar
or TimeZone to have a different hashCode.
None of the fields involved in the hashCode/equals should be
different. (Or I'm missing something about them).
Thanks, Roger
On 9/4/18 5:19 PM, Naoto Sato wrote:
Hi Roger,
I tentatively tried to return a shared zone within a cloned
Calendar. One test failed:
java/util/Calendar/CalendarRegression.Test4136399, where it makes
sure that the cloned Calendar object hash code should be different
for the better distribution. Although the comment does not reflect
the current implementation (getTimeZone() returning cloned zone),
the intention here seems to have the better hash distribution for
cloned objects.
Naoto
On 9/4/18 1:41 PM, Roger Riggs wrote:
Hi Naoto,
That access via reflection is going to go away sometime; so I'm not
too concerned about
maintaining compatibility of the internal implementation.
I think I'd rather see the memory savings, however small.
Let see if anyone else has a recommendation.
Roger
On 9/4/18 4:12 PM, Naoto Sato wrote:
Hi Roger,
Yes, I considered that too, but did not change the behavior and
just to maintain the field consistent. I agree that it would not
be observable via the public Calendar API, but some apps (like how
the submitter found it) may be doing something using reflection.
Naoto
On 9/4/18 12:31 PM, Roger Riggs wrote:
Hi Naoto,
The spec for clone doesn't say whether the clone should share or
not share the TimeZone.
Did you consider that if sharedZone was true , *not* to clone the
TimeZone?
It would still get cloned when requested from getTimeZone().
This does seem somewhat safer not to change the cloning behavior
but I don't think the behavior would be observable.
The current code and test is fine, except for reducing the
potential for sharing the TimeZone.
Thanks, Roger
On 9/4/2018 2:14 PM, Naoto Sato wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8210142
The proposed fix is located at:
http://cr.openjdk.java.net/~naoto/8210142/webrev.00/
The fix is a simple one line change, which is to make the
sharedZone field consistent with the cloned TimeZone instance in
Calendar.clone().
Naoto
