Thanks for the review. Updated patch is :
Please find some of my comments inlined.
On 30/11/16 10:17 PM, Roger Riggs wrote:
- line 172-177, might be a good place to use the new
I could have replaced those lines with :
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.
I am in touch with I18n SQE on writing tests for HOST Providers. But
testing scope, Golden data are yet to be discussed.
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.
If Masayoshi is satisfied and you have tested it in the target
perhaps it is not worthwhile to invest in a special case regression test.
On 11/30/2016 4:39 AM, Masayoshi Okutsu wrote:
Looks good to me.
On 11/22/2016 6:30 PM, Rachna Goel wrote:
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" .