Hi Rachna,
Ok, looks fine.
Thanks, Roger
On 12/1/2016 8:20 AM, Rachna Goel wrote:
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
