[15] RFR: 8246662: Test java/time/test/java/time/format/TestUnicodeExtension.java failed on japanese locale.

[Joe Wang]

On 6/9/2020 10:01 AM, naoto.sato at oracle.com wrote:
> Hi Roger,
>
> Thanks for the review.
>
> On 6/9/20 8:52 AM, Roger Riggs wrote:
>> Hi Naoto,
>>
>> Since the default locale is being changed even briefly, the test 
>> should be run in /othervm.
>> Add:  @run testng/othervm ...
>
> All java/time/test/java/time/format tests are run in othervm mode by 
> default, defined in java/time/test/TEST.properties.
>
>>
>> DateTimeFormatter:1498:
>>     Can the optimization be retained in the case where z.equals(zone)?
>>     Move it down to 1508+ and check the zone vs z.
>
> It's not only zone, but also has to check chrono and decimalStyle. In 
> the previous version, I intentionally did not keep the optimization, 
> as it would no longer be "fast" since it would need to check those 
> equality, but at least it would save creating the new 
> DateTimeFormatter instance, so I resurrected it. Here is the updated 
> webrev:
>
> http://cr.openjdk.java.net/~naoto/8246662/webrev.01/

Looks fine to me for the implementation of the method.

It's interesting to observe that the withXXXX methods, as demonstrated 
in the test, always create a new DateTimeFormatter instance. The Javadoc 
also said so, e.g. "Returns a copy of this formatter with a new XXXX", 
and by "a copy" it meant a new instance. Now the localizedBy method also 
"Returns a copy of this formatter", but it may actually returns this 
formatter.  Would it confuse users (into thinking they always get a new 
instance)?  Would we need to amend the javadoc a bit, e.g. indicating it 
returns this formatter if all of the above aspects are the same? (or 
otherwise a new instance is returned?)

The localizedBy method seems to me it's not "Unlike the withLocale 
method" in terms of "producing a different formatter". The difference 
lies in the attributes demonstrated in this changeset, e.g. zone, chrono 
and decimalStyle will be from the specified locale in localizedBy, while 
with the withLocale method, they are inherited. In other words, the 
withXXXX methods only changes the specified field.

-Joe

>
> Naoto
>
>>
>> Thanks, Roger
>>
>>
>>
>> On 6/8/20 5:06 PM, naoto.sato at oracle.com wrote:
>>> Hello,
>>>
>>> Please review the fix to the following issue:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8246662
>>>
>>> The proposed changeset is located at:
>>>
>>> https://cr.openjdk.java.net/~naoto/8246662/webrev.00/
>>>
>>> This is a regression caused by the fix to JDK-8244245, where the 
>>> locale related fields in the formatter are now overridden by 
>>> localizedBy() method. It was only comparing the locale object for 
>>> the fast path, but it was not sufficient.
>>>
>>> Naoto
>>