Hi Roger,

Thanks for the review. Updated patch is : http://cr.openjdk.java.net/~rgoel/JDK-8075577/webrev.02/


Please find some of my comments inlined.


On 30/11/16 10:17 PM, Roger Riggs wrote:
Hi Rachna,

macosx//HostLocaleProviderAdapterImpl.java:

- line 172-177, might be a good place to use the new ConcurrentMap.computeIfAbsent
I could have replaced those lines with :
dateFormatPatternsMap.computeIfAbsent(locale,
k -> new SoftReference<>(new AtomicReferenceArray<>(5 * 5)));

But, there are two check made on line no 173. (ref == null ) which will be checked by computeIfAbsent, But about second (dateFormatPatterns = ref.get()) == null)
will not be checked,  if those lines replaced.

- line 197: you could pre-size the StringBuilder since the target length can be estimated
   (unless they are all less than the default 16)
pre-sized it with initial " jrepattern" string.

macosx && windows//HostLocaleProviderAdapterImpl.java
- toJavaTimeDateTimePattern() - is there a way to avoid having two copies of this function?
    Perhaps as a static method in JavaTimeDateTimePatternImpl.java.
There seems to be no way to avoid having two copies.
Implementations of "HostLocaleProviderAdapterImpl" for different HOST Environments are loaded at run time by HOSTLocaleProviderAdapter. JavaTimeDateTimePatternImpl is an implementation of "JavaTimeDateTimePatternProvider" for JRE LocaleProviderAdapter.


The noreg-hard label on issue indicates testing is difficult and specific to OS and host configuration but it also seems unusual to have this much code and not have a regression test.

I am in touch with I18n SQE on writing tests for HOST Providers. But testing scope, Golden data are yet to be discussed.
If Masayoshi is satisfied and you have tested it in the target configuration then
perhaps it is not worthwhile to invest in a special case regression test.

Thanks, Roger

On 11/30/2016 4:39 AM, Masayoshi Okutsu wrote:
Looks good to me.

Masayoshi


On 11/22/2016 6:30 PM, Rachna Goel wrote:
Hi,

Please review fix for JDK-8075577.

Bug : https://bugs.openjdk.java.net/browse/JDK-8075577

webrev : http://cr.openjdk.java.net/~rgoel/JDK-8075577/webrev.01/

Fix is to introduce new private spi "sun.text.spi.JavaTimeDateTimePatternProvider.java" to retrieve LocaleProvider specific Date/Time Patterns for "java.time" .

Thanks,
Rachna






Reply via email to