Re: [9] RFR: 8008577: Use CLDR Locale Data by Default (JEP-252)
Looks good to me. Masayoshi On 6/25/2015 1:15 AM, Naoto Sato wrote: Thanks. Here is the diff from "webrev.01" to address your comment: http://hg.openjdk.java.net/jdk9/sandbox/jdk/rev/b8faab65bb62 Naoto On 6/24/15 2:16 AM, Masayoshi Okutsu wrote: applyParentLocales() sets parentLocalesMap before populating the map with data. It's possible that other threads look up the map without the (full) data. So, a Map (local variable) should be populated and then parentLocalesMap should be set to the Map. Also, parentLocalesMap needs to be volatile or synchronized should be used when setting it. I don't think it's necessary to use ConcurrentHashMap if the map is not modified after its initialization. Otherwise, the changes look good to me. Masayoshi On 6/24/2015 6:56 AM, Naoto Sato wrote: Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8008577/webrev.01/ As to the 3rd comment below, I did not modify it because that would simply duplicate the same piece of code in each getCandidateLocales() implementation (from the current location). Naoto On 6/19/15 1:53 AM, Masayoshi Okutsu wrote: Sorry for taking time. Here are my comments. src/jdk.localedata/share/classes/sun/text/resources/*JavaTimeSupplementary*.java: - The year range of the first line of the copyright header should be "2015," for the new ones and "2013, 2015," for the updated ones. - I wonder if JavaTimeSupplementary*.java contain some CR-LF lines because some line spacing is wide in the webrev. (hard to determine that from the webrev.) src/java.base/share/classes/sun/util/cldr/CLDRLocaleProviderAdapter.java: - applyParentLocales() needs to be thread-safe with parentLocalesMap? src/java.base/share/classes/sun/util/resources/LocaleData.java: - I wonder if the ResourceBundleBasedAdapter.getCandidateLocales() implementations should return an optimized candidate list rather than removing unsupported Locales from the list there. test/sun/text/resources/LocaleDataTest.java: + * -cldr option specifies to test CLDR locale data. The default data file name for this + * option is "CLDRLocaleData". - The file name should be "LocaleData.cldr"? test/java/text/Format/DecimalFormat/RoundingAndPropertyTest.java: - added the bug id, but no changes to the test? Thanks, Masayoshi On 6/9/2015 5:58 AM, Naoto Sato wrote: Hello, Please review the proposed changes for 8008577[1], the implementation of the JEP-252[2]. The proposed changes are located at: http://cr.openjdk.java.net/~naoto/8008577/webrev.00/ Here are the very high level summary of changes: - Now the default locale provider order is CLDR,JRE,SPI. - The CLDR data is upgraded from 21.0.1 to 27.0.0 - According the CLDR upgrade, the converter tool and the runtime are now capable of "alias" and "parentLocales" tags. - Regression tests that are specific to the existing JRE locale data are now run specifically with "JRE,SPI" providers. I would like the build group to review the build changes mainly with the CLDR changes, and i18n group for the rest. Naoto -- [1]: https://bugs.openjdk.java.net/browse/JDK-8008577 [2]: https://bugs.openjdk.java.net/browse/JDK-8043554
Re: [9] RFR: 8008577: Use CLDR Locale Data by Default (JEP-252)
Thanks. Here is the diff from "webrev.01" to address your comment: http://hg.openjdk.java.net/jdk9/sandbox/jdk/rev/b8faab65bb62 Naoto On 6/24/15 2:16 AM, Masayoshi Okutsu wrote: applyParentLocales() sets parentLocalesMap before populating the map with data. It's possible that other threads look up the map without the (full) data. So, a Map (local variable) should be populated and then parentLocalesMap should be set to the Map. Also, parentLocalesMap needs to be volatile or synchronized should be used when setting it. I don't think it's necessary to use ConcurrentHashMap if the map is not modified after its initialization. Otherwise, the changes look good to me. Masayoshi On 6/24/2015 6:56 AM, Naoto Sato wrote: Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8008577/webrev.01/ As to the 3rd comment below, I did not modify it because that would simply duplicate the same piece of code in each getCandidateLocales() implementation (from the current location). Naoto On 6/19/15 1:53 AM, Masayoshi Okutsu wrote: Sorry for taking time. Here are my comments. src/jdk.localedata/share/classes/sun/text/resources/*JavaTimeSupplementary*.java: - The year range of the first line of the copyright header should be "2015," for the new ones and "2013, 2015," for the updated ones. - I wonder if JavaTimeSupplementary*.java contain some CR-LF lines because some line spacing is wide in the webrev. (hard to determine that from the webrev.) src/java.base/share/classes/sun/util/cldr/CLDRLocaleProviderAdapter.java: - applyParentLocales() needs to be thread-safe with parentLocalesMap? src/java.base/share/classes/sun/util/resources/LocaleData.java: - I wonder if the ResourceBundleBasedAdapter.getCandidateLocales() implementations should return an optimized candidate list rather than removing unsupported Locales from the list there. test/sun/text/resources/LocaleDataTest.java: + * -cldr option specifies to test CLDR locale data. The default data file name for this + * option is "CLDRLocaleData". - The file name should be "LocaleData.cldr"? test/java/text/Format/DecimalFormat/RoundingAndPropertyTest.java: - added the bug id, but no changes to the test? Thanks, Masayoshi On 6/9/2015 5:58 AM, Naoto Sato wrote: Hello, Please review the proposed changes for 8008577[1], the implementation of the JEP-252[2]. The proposed changes are located at: http://cr.openjdk.java.net/~naoto/8008577/webrev.00/ Here are the very high level summary of changes: - Now the default locale provider order is CLDR,JRE,SPI. - The CLDR data is upgraded from 21.0.1 to 27.0.0 - According the CLDR upgrade, the converter tool and the runtime are now capable of "alias" and "parentLocales" tags. - Regression tests that are specific to the existing JRE locale data are now run specifically with "JRE,SPI" providers. I would like the build group to review the build changes mainly with the CLDR changes, and i18n group for the rest. Naoto -- [1]: https://bugs.openjdk.java.net/browse/JDK-8008577 [2]: https://bugs.openjdk.java.net/browse/JDK-8043554
Re: [9] RFR: 8008577: Use CLDR Locale Data by Default (JEP-252)
applyParentLocales() sets parentLocalesMap before populating the map with data. It's possible that other threads look up the map without the (full) data. So, a Map (local variable) should be populated and then parentLocalesMap should be set to the Map. Also, parentLocalesMap needs to be volatile or synchronized should be used when setting it. I don't think it's necessary to use ConcurrentHashMap if the map is not modified after its initialization. Otherwise, the changes look good to me. Masayoshi On 6/24/2015 6:56 AM, Naoto Sato wrote: Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8008577/webrev.01/ As to the 3rd comment below, I did not modify it because that would simply duplicate the same piece of code in each getCandidateLocales() implementation (from the current location). Naoto On 6/19/15 1:53 AM, Masayoshi Okutsu wrote: Sorry for taking time. Here are my comments. src/jdk.localedata/share/classes/sun/text/resources/*JavaTimeSupplementary*.java: - The year range of the first line of the copyright header should be "2015," for the new ones and "2013, 2015," for the updated ones. - I wonder if JavaTimeSupplementary*.java contain some CR-LF lines because some line spacing is wide in the webrev. (hard to determine that from the webrev.) src/java.base/share/classes/sun/util/cldr/CLDRLocaleProviderAdapter.java: - applyParentLocales() needs to be thread-safe with parentLocalesMap? src/java.base/share/classes/sun/util/resources/LocaleData.java: - I wonder if the ResourceBundleBasedAdapter.getCandidateLocales() implementations should return an optimized candidate list rather than removing unsupported Locales from the list there. test/sun/text/resources/LocaleDataTest.java: + * -cldr option specifies to test CLDR locale data. The default data file name for this + * option is "CLDRLocaleData". - The file name should be "LocaleData.cldr"? test/java/text/Format/DecimalFormat/RoundingAndPropertyTest.java: - added the bug id, but no changes to the test? Thanks, Masayoshi On 6/9/2015 5:58 AM, Naoto Sato wrote: Hello, Please review the proposed changes for 8008577[1], the implementation of the JEP-252[2]. The proposed changes are located at: http://cr.openjdk.java.net/~naoto/8008577/webrev.00/ Here are the very high level summary of changes: - Now the default locale provider order is CLDR,JRE,SPI. - The CLDR data is upgraded from 21.0.1 to 27.0.0 - According the CLDR upgrade, the converter tool and the runtime are now capable of "alias" and "parentLocales" tags. - Regression tests that are specific to the existing JRE locale data are now run specifically with "JRE,SPI" providers. I would like the build group to review the build changes mainly with the CLDR changes, and i18n group for the rest. Naoto -- [1]: https://bugs.openjdk.java.net/browse/JDK-8008577 [2]: https://bugs.openjdk.java.net/browse/JDK-8043554
Re: [9] RFR: 8008577: Use CLDR Locale Data by Default (JEP-252)
Here is the updated webrev: http://cr.openjdk.java.net/~naoto/8008577/webrev.01/ As to the 3rd comment below, I did not modify it because that would simply duplicate the same piece of code in each getCandidateLocales() implementation (from the current location). Naoto On 6/19/15 1:53 AM, Masayoshi Okutsu wrote: Sorry for taking time. Here are my comments. src/jdk.localedata/share/classes/sun/text/resources/*JavaTimeSupplementary*.java: - The year range of the first line of the copyright header should be "2015," for the new ones and "2013, 2015," for the updated ones. - I wonder if JavaTimeSupplementary*.java contain some CR-LF lines because some line spacing is wide in the webrev. (hard to determine that from the webrev.) src/java.base/share/classes/sun/util/cldr/CLDRLocaleProviderAdapter.java: - applyParentLocales() needs to be thread-safe with parentLocalesMap? src/java.base/share/classes/sun/util/resources/LocaleData.java: - I wonder if the ResourceBundleBasedAdapter.getCandidateLocales() implementations should return an optimized candidate list rather than removing unsupported Locales from the list there. test/sun/text/resources/LocaleDataTest.java: + * -cldr option specifies to test CLDR locale data. The default data file name for this + * option is "CLDRLocaleData". - The file name should be "LocaleData.cldr"? test/java/text/Format/DecimalFormat/RoundingAndPropertyTest.java: - added the bug id, but no changes to the test? Thanks, Masayoshi On 6/9/2015 5:58 AM, Naoto Sato wrote: Hello, Please review the proposed changes for 8008577[1], the implementation of the JEP-252[2]. The proposed changes are located at: http://cr.openjdk.java.net/~naoto/8008577/webrev.00/ Here are the very high level summary of changes: - Now the default locale provider order is CLDR,JRE,SPI. - The CLDR data is upgraded from 21.0.1 to 27.0.0 - According the CLDR upgrade, the converter tool and the runtime are now capable of "alias" and "parentLocales" tags. - Regression tests that are specific to the existing JRE locale data are now run specifically with "JRE,SPI" providers. I would like the build group to review the build changes mainly with the CLDR changes, and i18n group for the rest. Naoto -- [1]: https://bugs.openjdk.java.net/browse/JDK-8008577 [2]: https://bugs.openjdk.java.net/browse/JDK-8043554
Re: [9] RFR: 8008577: Use CLDR Locale Data by Default (JEP-252)
Sorry for taking time. Here are my comments. src/jdk.localedata/share/classes/sun/text/resources/*JavaTimeSupplementary*.java: - The year range of the first line of the copyright header should be "2015," for the new ones and "2013, 2015," for the updated ones. - I wonder if JavaTimeSupplementary*.java contain some CR-LF lines because some line spacing is wide in the webrev. (hard to determine that from the webrev.) src/java.base/share/classes/sun/util/cldr/CLDRLocaleProviderAdapter.java: - applyParentLocales() needs to be thread-safe with parentLocalesMap? src/java.base/share/classes/sun/util/resources/LocaleData.java: - I wonder if the ResourceBundleBasedAdapter.getCandidateLocales() implementations should return an optimized candidate list rather than removing unsupported Locales from the list there. test/sun/text/resources/LocaleDataTest.java: + * -cldr option specifies to test CLDR locale data. The default data file name for this + * option is "CLDRLocaleData". - The file name should be "LocaleData.cldr"? test/java/text/Format/DecimalFormat/RoundingAndPropertyTest.java: - added the bug id, but no changes to the test? Thanks, Masayoshi On 6/9/2015 5:58 AM, Naoto Sato wrote: Hello, Please review the proposed changes for 8008577[1], the implementation of the JEP-252[2]. The proposed changes are located at: http://cr.openjdk.java.net/~naoto/8008577/webrev.00/ Here are the very high level summary of changes: - Now the default locale provider order is CLDR,JRE,SPI. - The CLDR data is upgraded from 21.0.1 to 27.0.0 - According the CLDR upgrade, the converter tool and the runtime are now capable of "alias" and "parentLocales" tags. - Regression tests that are specific to the existing JRE locale data are now run specifically with "JRE,SPI" providers. I would like the build group to review the build changes mainly with the CLDR changes, and i18n group for the rest. Naoto -- [1]: https://bugs.openjdk.java.net/browse/JDK-8008577 [2]: https://bugs.openjdk.java.net/browse/JDK-8043554
Re: [9] RFR: 8008577: Use CLDR Locale Data by Default (JEP-252)
Thank you for the quick review, Erik! Naoto On 6/9/15 12:59 AM, Erik Joelsson wrote: Hello Naoto, Build changes look good to me. /Erik On 2015-06-08 22:58, Naoto Sato wrote: Hello, Please review the proposed changes for 8008577[1], the implementation of the JEP-252[2]. The proposed changes are located at: http://cr.openjdk.java.net/~naoto/8008577/webrev.00/ Here are the very high level summary of changes: - Now the default locale provider order is CLDR,JRE,SPI. - The CLDR data is upgraded from 21.0.1 to 27.0.0 - According the CLDR upgrade, the converter tool and the runtime are now capable of "alias" and "parentLocales" tags. - Regression tests that are specific to the existing JRE locale data are now run specifically with "JRE,SPI" providers. I would like the build group to review the build changes mainly with the CLDR changes, and i18n group for the rest. Naoto -- [1]: https://bugs.openjdk.java.net/browse/JDK-8008577 [2]: https://bugs.openjdk.java.net/browse/JDK-8043554
Re: [9] RFR: 8008577: Use CLDR Locale Data by Default (JEP-252)
Hello Naoto, Build changes look good to me. /Erik On 2015-06-08 22:58, Naoto Sato wrote: Hello, Please review the proposed changes for 8008577[1], the implementation of the JEP-252[2]. The proposed changes are located at: http://cr.openjdk.java.net/~naoto/8008577/webrev.00/ Here are the very high level summary of changes: - Now the default locale provider order is CLDR,JRE,SPI. - The CLDR data is upgraded from 21.0.1 to 27.0.0 - According the CLDR upgrade, the converter tool and the runtime are now capable of "alias" and "parentLocales" tags. - Regression tests that are specific to the existing JRE locale data are now run specifically with "JRE,SPI" providers. I would like the build group to review the build changes mainly with the CLDR changes, and i18n group for the rest. Naoto -- [1]: https://bugs.openjdk.java.net/browse/JDK-8008577 [2]: https://bugs.openjdk.java.net/browse/JDK-8043554
