RFR: 8333755: NumberFormat integer only parsing breaks when format has suffix
Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8333920)
which corrects a bug where NumberFormat cannot successfully parse values in
integer only mode, when the format expects a suffix.
For example,
// a format that expects a currency suffix
var fmt = NumberFormat.getCurrencyInstance(Locale.FRANCE);
fmt.setParseIntegerOnly(true);
failFmt.parse("5,00 €"); // throws ParseException when you would have expected
5 returned
When parsing in integer only mode, instead of breaking upon a decimal symbol
encounter, we should store the index but continue to fully parse so that we can
verify the entire string and increment our position to search for and match the
suffix. Upon a successful suffix match, we can then set `ParsePosition.index`
back to the stored decimal index.
It should be noted that CompactNumberFormat did previously have code that would
traverse the rest of the String, to allow matching of the suffix. The
difference is that NumberFormat returns the index upon decimal symbol
encounter, while CompactNumberFormat was implemented to return the index
reflected by the entire string traversal. Such differences cannot be
standardized due to behavioral compatibility concerns.
Thus while parsing in integer only, CNF sets index to the `Position.fullPos()`,
while DF sets index to the `Position.intPos()`.
-
Commit messages:
- init
Changes: https://git.openjdk.org/jdk/pull/19664/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19664=00
Issue: https://bugs.openjdk.org/browse/JDK-8333755
Stats: 180 lines in 5 files changed: 85 ins; 31 del; 64 mod
Patch: https://git.openjdk.org/jdk/pull/19664.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19664/head:pull/19664
PR: https://git.openjdk.org/jdk/pull/19664
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v2]
On Tue, 11 Jun 2024 17:15:49 GMT, Naoto Sato wrote: >> Yeah, I believe you're right. Noted as something else to file against the >> translation tool. > > Just out of curiosity, do we know why `\t` was attached in the first place? I > don't think it is with the original English properties. I can see that in 2012, the original did not have `\t` while the translated files did. (Did not search before then). Seems like this discrepancy originates from awhile ago. - PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1635242656
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update
On Fri, 7 Jun 2024 22:46:44 GMT, Damon Nguyen wrote:
> This issue is responsible for updating the translations of all the
> localize(able) resources in the JDK. Primarily, the changes between JDK 22
> RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated.
>
> The translation tool adjusted some definitions, which causes some changes in
> localized files where the source file had no changes. This causes some words
> being reverted from localized languages to English, and some had its
> definitions changed.
>
> Alternatively, the diffs are viewable here and was generated using John
> Gibbon's diff tool for l10n:
> https://cr.openjdk.org/~dnguyen/output2/
Cannot speak to the translations themselves, but in general the changes LGTM
pending the comments.
src/java.desktop/share/classes/sun/print/resources/serviceui_de.properties line
1:
> 1: #
It looks like these .properties files need to have their copyright years
bumped. I believe since the original was not bumped, the translation tool did
not reflect the correct year in the localized versions.
src/java.desktop/share/classes/sun/print/resources/serviceui_zh_CN.properties
line 66:
> 64: label.size=大小():
> 65: label.source=来源():
> 66: label.outputbins=输出托盘()：
Looks like the translation tool changed this from a colon (U+003A) to a full
width colon (U+FF1A). There are l10n rules when it comes to punctuation, I am
not sure if this is a result of that. It looks like the surrounding key/values
are simply using (U+003A) colons. If we decide to revert this colon change, we
need to also probably file a bug against the translation tool.
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/msg/XMLMessages_de.properties
line 278:
> 276: UndeclaredElementInContentSpec = Contentmodell des Elements
> "{0}" verweist auf das nicht deklarierte Element "{1}".
> 277: UniqueNotationName = Deklaration für die Notation "{0}" ist
> nicht eindeutig. Ein jeweiliger Name darf nicht in mehreren
> Notationsdeklarationen deklariert werden.
> 278: ENTITYFailedInitializeGrammar = ENTITYDatatype-Validator: Nicht
> erfolgreich. Initialisierungsmethode muss mit einer gültigen
> Grammatikreferenz aufgerufen werden. \t
It looks like the _zh_CN_ file should also have the `\t` removed, but such
changes are done by the translation tool. I think in this case, we should
manually remove it, and then file a bug against the translation tool.
src/jdk.jconsole/share/classes/sun/tools/jconsole/resources/messages_de.properties
line 285:
> 283: VIRTUAL_MACHINE=Virtuelle Maschine
> 284: VM_ARGUMENTS=VM-Argumente
> 285: VMINTERNAL_FRAME_ACCESSIBLE_DESCRIPTION=Innerer Frame für das Monitoring
> einer Java Virtual Machine
This one-off change is consistent with the change in
_src/jdk.jdi/share/classes/com/sun/tools/example/debug/tty/TTYResources_de.java_.
src/jdk.jshell/share/classes/jdk/internal/jshell/tool/resources/l10n_de.properties
line 183:
> 181: jshell.fix.wrong.shortcut =Unerwartetes Zeichen nach
> Umschalt+Tab.\nVerwenden Sie "I" für automatischen Import, "V" zur
> Variablenerstellung oder "M" zur Methodenerstellung.\nWeitere Informationen
> finden Sie unter:\n/help shortcuts
> 182:
> 183: help.usage = Verwendung: jshell ... ...\nMögliche
> Optionen:\n--class-pathGibt an, wo die Benutzerklassendateien
> gespeichert sind\n--module-path Gibt an, wo die Anwendungsmodule
> gespeichert sind\n--add-modules (,)*\n
> Gibt aufzulösende Module oder alle Module im\n
> Modulpfad an, wenn ALL-MODULE-PATHs lautet\n
> --enable-native-access\n Ermöglicht Ausführung
> eingeschränkter nativer Methoden durch Code\n--enable-preview Code
> kann Vorschaufeatures in diesem Release nutzen\n--startup
> Ersetzung der Startdefinitionen mit einer Ausführung\n--no-startup
> Startdefinitionen werden nicht ausgeführt\n--feedback Gibt
> den anfänglichen Feedbackmodus an. Der Modus kann\n
> vordefiniert (Silent, Concise, Normal oder Verbose) oder\n
> v
orab benutzerdefiniert sein\n-qStilles Feedback.
Identisch mit: --feedback concise\n-sÄußerst stilles
Feedback. Identisch mit: --feedback silent\n-v
Verbose-Feedback. Identisch mit: --feedback verbose\n-J
Übergibt an das Laufzeitsystem, hat aber keine Auswirkungen\n
auf die Ausführung von Code-Snippets. Um Kennzeichen
anzugeben,\ndie die Ausführung von Code-Snippets
beeinflussen, verwenden Sie\n-R. Verwenden
Sie alternativ dazu -J mit\n--execution
local.\n-R Übergibt nur dann an das
Laufzeitsystem, wenn\nCode-Snippets
Integrated: 8333456: CompactNumberFormat integer parsing fails when string has no suffix
On Mon, 3 Jun 2024 22:32:54 GMT, Justin Lu wrote:
> Please review this PR which handles incorrect CompactNumberFormat integer
> only parsing when there is no suffix.
>
> See the following snippet,
>
>
> var fmt = NumberFormat.getCompactNumberInstance(Locale.US,
> NumberFormat.Style.SHORT)
> fmt.setParseIntegerOnly(true)
> fmt.parse("5K") // returns 5000
> fmt.parse("50.00") // returns 50
> fmt.parse("5") // unexpectedly throws StringIndexOutOfBoundsException
> fmt.parse("5 ") // returns 5
>
>
> Within the `parse` method, there is code that advances `position` past the
> fractional portion to find the suffix when `parseIntegerOnly()` is true.
> However, if a valid string input is given with no suffix,
> `DecimalFormat.subparseNumber()` will fully iterate through the string and
> `position` will be equal to the length of the string, throwing
> StringIndexOutOfBoundsException when `charAt` is invoked (line 1740).
>
> We should check to make sure position does not exceed the string length
> before deciding to check for a decimal symbol.
This pull request has now been integrated.
Changeset: 6238bc8d
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/6238bc8da2abe7a1f0cdd98c0af01e9ba1869ec3
Stats: 26 lines in 2 files changed: 24 ins; 0 del; 2 mod
8333456: CompactNumberFormat integer parsing fails when string has no suffix
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/19533
Re: RFR: 8333456: CompactNumberFormat integer parsing fails when string has no suffix [v2]
> Please review this PR which handles incorrect CompactNumberFormat integer
> only parsing when there is no suffix.
>
> See the following snippet,
>
>
> var fmt = NumberFormat.getCompactNumberInstance(Locale.US,
> NumberFormat.Style.SHORT)
> fmt.setParseIntegerOnly(true)
> fmt.parse("5K") // returns 5000
> fmt.parse("50.00") // returns 50
> fmt.parse("5") // unexpectedly throws StringIndexOutOfBoundsException
> fmt.parse("5 ") // returns 5
>
>
> Within the `parse` method, there is code that advances `position` past the
> fractional portion to find the suffix when `parseIntegerOnly()` is true.
> However, if a valid string input is given with no suffix,
> `DecimalFormat.subparseNumber()` will fully iterate through the string and
> `position` will be equal to the length of the string, throwing
> StringIndexOutOfBoundsException when `charAt` is invoked (line 1740).
>
> We should check to make sure position does not exceed the string length
> before deciding to check for a decimal symbol.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
move test to existing test file
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19533/files
- new: https://git.openjdk.org/jdk/pull/19533/files/b6bb7e56..accb1850
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19533=01
- incr: https://webrevs.openjdk.org/?repo=jdk=19533=00-01
Stats: 93 lines in 2 files changed: 24 ins; 68 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19533.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19533/head:pull/19533
PR: https://git.openjdk.org/jdk/pull/19533
Re: RFR: 8333462: Performance regression of new DecimalFormat() when compare to jdk11
On Tue, 4 Jun 2024 02:32:28 GMT, lingjun-cg wrote:
> Run the below benchmark test ,it show the average time of new
> DecimalFormat() increase 18% when compare to jdk 11.
>
> the result with jdk 11:
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 248.300 ? 5.158 ns/op
>
>
> the result with current jdk:
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 303.381 ? 5.252 ns/op
>
>
>
> @BenchmarkMode(Mode.AverageTime)
> @Warmup(iterations = 5, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @Measurement(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
> @State(Scope.Thread)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> public class JmhDecimalFormat {
>
> @Setup(Level.Trial)
> public void setup() {
> }
>
> @Benchmark
> public void testNewOnly() throws InterruptedException {
> new DecimalFormat("#0.0");
> }
> }
>
>
> Compare the flame graph it shows the
> java.text.DecimalFormatSymbols#findNonFormatChar takes a significant time.
> After replacing the lambda implementation with a simple loop , it shows
> nearly the same performance as jdk 11.
>
>
> Benchmark Mode CntScore Error Units
> JmhDecimalFormat.testNewOnly avgt 50 209.874 ? 9.951 ns/op
>
>
> [flame-graph-jdk11-jdk21.zip](https://github.com/user-attachments/files/15541764/flame-graph-jdk11-jdk21.zip)
LGTM. Also needs copyright year update
-
Marked as reviewed by jlu (Committer).
PR Review: https://git.openjdk.org/jdk/pull/19534#pullrequestreview-2097304072
RFR: 8333456: CompactNumberFormat integer parsing fails when string has no suffix
Please review this PR which handles incorrect CompactNumberFormat integer only
parsing when there is no suffix.
See the following snippet,
var fmt = NumberFormat.getCompactNumberInstance(Locale.US,
NumberFormat.Style.SHORT)
fmt.setParseIntegerOnly(true)
fmt.parse("5K") // returns 5000
fmt.parse("50.00") // returns 50
fmt.parse("5") // unexpectedly throws StringIndexOutOfBoundsException
fmt.parse("5 ") // returns 5
Within the `parse` method, there is code that advances `position` past the
fractional portion to find the suffix when `parseIntegerOnly()` is true.
However, if a valid string input is given with no suffix,
`DecimalFormat.subparseNumber()` will fully iterate through the string and
`position` will be equal to the length of the string, throwing
StringIndexOutOfBoundsException when `charAt` is invoked (line 1740).
We should check to make sure position does not exceed the string length before
deciding to check for a decimal symbol.
-
Commit messages:
- init
Changes: https://git.openjdk.org/jdk/pull/19533/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19533=00
Issue: https://bugs.openjdk.org/browse/JDK-8333456
Stats: 69 lines in 2 files changed: 68 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19533.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19533/head:pull/19533
PR: https://git.openjdk.org/jdk/pull/19533
Integrated: 8332424: Update IANA Language Subtag Registry to Version 2024-05-16
On Fri, 17 May 2024 18:04:02 GMT, Justin Lu wrote: > Please review this PR which is a trivial update to the IANA sub tag registry > to version 2024-05-16. Locale tests pass as expected after update. > > Associated announcement -> > https://mm.icann.org/pipermail/ietf-languages-announcements/2024-May/91.html This pull request has now been integrated. Changeset: 6dac8d64 Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/6dac8d64527b4e9ade783b99f82fbecd81c426a6 Stats: 9 lines in 2 files changed: 6 ins; 0 del; 3 mod 8332424: Update IANA Language Subtag Registry to Version 2024-05-16 Reviewed-by: naoto, iris - PR: https://git.openjdk.org/jdk/pull/19286
Integrated: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent
On Fri, 3 May 2024 08:47:03 GMT, Justin Lu wrote:
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
This pull request has now been integrated.
Changeset: ffb0867e
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/ffb0867e2c07b41cb7124e11fe6cf63d9471f0d2
Stats: 269 lines in 4 files changed: 254 ins; 2 del; 13 mod
8331485: Odd Results when Parsing Scientific Notation with Large Exponent
8331680: NumberFormat is missing some bad exponent strict parse cases
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/19075
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v6]
On Fri, 17 May 2024 21:59:38 GMT, Justin Lu wrote:
>> Please review this PR which corrects an edge case bug for
>> java.text.DecimalFormat that causes incorrect parsing results for strings
>> with very large exponent values.
>>
>> When parsing values with large exponents, if the value of the exponent
>> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value
>> of the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
>> mantissa. Both results are confusing and incorrect.
>>
>> For example,
>>
>>
>> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
>> fmt.parse(".1E2147483648"); // returns 0.0
>> fmt.parse(".1E9223372036854775808"); // returns 0.1
>> // For comparison
>> Double.parseDouble(".1E2147483648"); // returns Infinity
>> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>>
>>
>> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
>
> Justin Lu has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - change impl to support whitebox test
> - adjust impl to increase accuracy when decimalAt/exponent value are both
> around Integer.MAX/MIN
Thanks for the reviews; tests pass after merge with latest master.
-
PR Comment: https://git.openjdk.org/jdk/pull/19075#issuecomment-2140780684
RFR: 8331342: Convert Base64 tests to JUnit
Please review this test-only clean up PR which converts the java.util.Base64 tests to run under JUnit. In general, this allows for the tests to run independently, separates the data providers from the tests, as well being able to utilize the built in JUnit utility test methods to reduce boilerplate testing code. - Commit messages: - Additional clarifying comments - convert TestBase64.java - convert TestBase64Golden.java - convert Base64GetEncoderTest.java Changes: https://git.openjdk.org/jdk/pull/19337/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19337=00 Issue: https://bugs.openjdk.org/browse/JDK-8331342 Stats: 823 lines in 3 files changed: 257 ins; 336 del; 230 mod Patch: https://git.openjdk.org/jdk/pull/19337.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19337/head:pull/19337 PR: https://git.openjdk.org/jdk/pull/19337
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v6]
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
Justin Lu has updated the pull request incrementally with two additional
commits since the last revision:
- change impl to support whitebox test
- adjust impl to increase accuracy when decimalAt/exponent value are both
around Integer.MAX/MIN
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19075/files
- new: https://git.openjdk.org/jdk/pull/19075/files/2c167493..7fe44600
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19075=05
- incr: https://webrevs.openjdk.org/?repo=jdk=19075=04-05
Stats: 90 lines in 2 files changed: 65 ins; 12 del; 13 mod
Patch: https://git.openjdk.org/jdk/pull/19075.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19075/head:pull/19075
PR: https://git.openjdk.org/jdk/pull/19075
RFR: 8332424: Update IANA Language Subtag Registry to Version 2024-05-16
Please review this PR which is a trivial update to the IANA sub tag registry to version 2024-05-16. Locale tests pass as expected after update. Associated announcement -> https://mm.icann.org/pipermail/ietf-languages-announcements/2024-May/91.html - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/19286/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19286=00 Issue: https://bugs.openjdk.org/browse/JDK-8332424 Stats: 9 lines in 2 files changed: 6 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/19286.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19286/head:pull/19286 PR: https://git.openjdk.org/jdk/pull/19286
Re: RFR: 8331866: Add warnings for locale data dependence
On Wed, 8 May 2024 20:21:50 GMT, Naoto Sato wrote: > In order to enlighten users to not depend on a particular version of the > locale data, putting a note into the JDK vs. CLDR version chart would be > appropriate. LGTM. Wording in the beginning of the class spec makes it clear that locale sensitive factory methods are dependent on the LSP. By now adding wording that the CLDR data changes, it should hopefully help to reduce the expectation from users of consistent data between releases for these locale sensitive methods. - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/19145#pullrequestreview-2046636580
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v4]
On Mon, 6 May 2024 19:47:48 GMT, Axel Hauschulte wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Check both parse methods
>
> test/jdk/java/text/Format/DecimalFormat/LargeExponentsTest.java line 150:
>
>> 148: // Long.MIN_VALUE
>> 149: Arguments.of("1.23E-9223372036854775808", 0.0)
>> 150: );
>
> I would suggest adding one more test case to the edge cases:
>
> Arguments.of("0.0123E-2147483648", 0.0)
>
> This will test the adjustment of the `digits.decimalAt` field for an exponent
> that is within the range of integer, but due to the mantissa not being in its
> standardized form an overflow will occure non the less.
Right, that's a good test case to have, (which exposed a needed update in the
implementation).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1591971646
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v5]
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
correct behavior when underflow from adjustment + use MIN
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19075/files
- new: https://git.openjdk.org/jdk/pull/19075/files/17a3b3aa..2c167493
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19075=04
- incr: https://webrevs.openjdk.org/?repo=jdk=19075=03-04
Stats: 19 lines in 2 files changed: 9 ins; 4 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/19075.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19075/head:pull/19075
PR: https://git.openjdk.org/jdk/pull/19075
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v4]
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
Check both parse methods
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19075/files
- new: https://git.openjdk.org/jdk/pull/19075/files/25782781..17a3b3aa
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19075=03
- incr: https://webrevs.openjdk.org/?repo=jdk=19075=02-03
Stats: 15 lines in 1 file changed: 14 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/19075.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19075/head:pull/19075
PR: https://git.openjdk.org/jdk/pull/19075
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]
On Sun, 5 May 2024 20:51:10 GMT, Axel Hauschulte wrote:
>> Justin Lu has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - correct other test comment
>> - reflect review
>
> Hello, I filed [JDK-8331485](https://bugs.openjdk.org/browse/JDK-8331485).
> Thank you for addressing this bug so quickly.
>
> I have a thought/concern regarding the handling of exponents that exceed
> `Long.MAX_VALUE` in this PR:
>
>> If the value of the exponent exceeds `Long.MAX_VALUE`, the parsed value is
>> equal to the mantissa. Both results are confusing and incorrect.
>>
>> For example,
>>
>> ```
>> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
>> fmt.parse(".1E2147483648"); // returns 0.0
>> fmt.parse(".1E9223372036854775808"); // returns 0.1
>> // For comparison
>> Double.parseDouble(".1E2147483648"); // returns Infinity
>> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>> ```
>
> The method [`parse(String,
> ParsePosition)`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/text/DecimalFormat.html#parse(java.lang.String,java.text.ParsePosition))
> uses the
> [`ParsePosition`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/text/ParsePosition.html)
> object as an input and output parameter to determine at what position the
> parsing should start as well as to communicate up to which position the input
> string has been consumed during the parsing. (This can be very handy if you
> use different
> [`Format`](https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/text/Format.html)s
> to parse through a string.)
>
> For example, if there is a method like this
>
> static void parseNumber(String s) {
> NumberFormat numberFormat = NumberFormat.getInstance(Locale.US);
> ParsePosition parsePosition = new ParsePosition(0);
> Number parseResult = numberFormat.parse(s, parsePosition);
> System.out.println(STR."numberFormat.parse("{s}") -> {parseResult};
> parsePosition: {parsePosition}");
> }
>
> `parseNumber("0.123E1XYZ")` will parse the provided string from the beginning
> to position 7, ignoring the letters at the end of the string. The resulting
> `Double` value is therefore 1.23 and `parsePosition.getIndex()` returns 7.
>
> Having an exponent that exceeds `Long.MAX_VALUE`, for instance
> `parseNumber("0.123E9223372036854775808")`, the current implementation of
> `DecimalFormat` in JDK 22 does the following: Parse the provided string from
> the beginning to position 5, ignoring the exponent (because it is too long).
> The resulting `Double` is therefore 0.123 and `parsePosition.getIndex()`
> returns 5.
>
> The solution implemented in this PR would produce a parsing result of
> `Double.POSITIVE_INFINITY`, however, `parsePosition.getIndex()` would sti...
Hi @ahauschulte, thanks for bringing up your concern, that's a good catch.
As you stated, I agree that the latter solution would be the ideal one here.
The parse position index should reflect the actual exponent consumed, even if
the exponent exceeds `Long.MAX_VALUE`.
In
https://github.com/openjdk/jdk/pull/19075/commits/25782781394dc7a2e0c39605515ab14be41649b0,
the behavior of parsing such an exponent value should now reflect this.
-
PR Comment: https://git.openjdk.org/jdk/pull/19075#issuecomment-2096592065
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v3]
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
Address PP for exp > Long.MAX_VALUE + more exp test cases
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19075/files
- new: https://git.openjdk.org/jdk/pull/19075/files/bc391f96..25782781
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19075=02
- incr: https://webrevs.openjdk.org/?repo=jdk=19075=01-02
Stats: 41 lines in 4 files changed: 28 ins; 5 del; 8 mod
Patch: https://git.openjdk.org/jdk/pull/19075.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19075/head:pull/19075
PR: https://git.openjdk.org/jdk/pull/19075
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]
On Fri, 3 May 2024 18:24:31 GMT, Justin Lu wrote: >> src/java.base/share/classes/java/text/DecimalFormat.java line 2596: >> >>> 2594: exponent = -exponent; >>> 2595: } >>> 2596: sawExponent = true; >> >> I see you removed this assignment. I am wondering if we need this variable >> at all. > > Should be updated in the latest commit, I had forgotten to remove the > assignment as well as the check. As we always `break` once we parse the > exponent, we have no need for the boolean flag. Thanks for the review! Perhaps there was a planned reason for it before, but as it stands it serves no purpose, so until there is a reason to have it, I think we can safely remove it to de-clutter the code. - PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589588844
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]
On Fri, 3 May 2024 18:02:05 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - correct other test comment >> - reflect review > > src/java.base/share/classes/java/text/DecimalFormat.java line 2596: > >> 2594: exponent = -exponent; >> 2595: } >> 2596: sawExponent = true; > > I see you removed this assignment. I am wondering if we need this variable at > all. Should be updated in the latest commit, I had forgotten to remove the assignment as well as the check. As we always `break` once we parse the exponent, we have no need for the boolean flag. Thanks for the review! - PR Review Comment: https://git.openjdk.org/jdk/pull/19075#discussion_r1589587379
Re: RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent [v2]
> Please review this PR which corrects an edge case bug for
> java.text.DecimalFormat that causes incorrect parsing results for strings
> with very large exponent values.
>
> When parsing values with large exponents, if the value of the exponent
> exceeds `Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of
> the exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the
> mantissa. Both results are confusing and incorrect.
>
> For example,
>
>
> NumberFormat fmt = NumberFormat.getInstance(Locale.US);
> fmt.parse(".1E2147483648"); // returns 0.0
> fmt.parse(".1E9223372036854775808"); // returns 0.1
> // For comparison
> Double.parseDouble(".1E2147483648"); // returns Infinity
> Double.parseDouble(".1E9223372036854775808"); // returns Infinity
>
>
> After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
Justin Lu has updated the pull request incrementally with two additional
commits since the last revision:
- correct other test comment
- reflect review
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19075/files
- new: https://git.openjdk.org/jdk/pull/19075/files/5bf5d6ef..bc391f96
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19075=01
- incr: https://webrevs.openjdk.org/?repo=jdk=19075=00-01
Stats: 9 lines in 2 files changed: 0 ins; 1 del; 8 mod
Patch: https://git.openjdk.org/jdk/pull/19075.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19075/head:pull/19075
PR: https://git.openjdk.org/jdk/pull/19075
Integrated: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory
On Wed, 1 May 2024 16:10:06 GMT, Justin Lu wrote: > Please review this PR which converts _TestEncodingDecodingLength.java_ into a > whitebox test which allows for testing to be done without memory usage issues. > > Currently, the test requires about ~2.75 `Integer.MAX_VALUE` sized byte > arrays worth of memory. (2 for the initial array allocation, .75 for the > target array in `decode()`). While the `-Xms6g -Xmx8g` options should address > this, there have been intermittent memory issues, as the underlying machine > machine may be running other tests simultaneously. > > By converting this test to a white-box test not only does it get rid of > memory issues, but it also gets rid of the need to decode 2GB of data 3 > times. The change is done using reflection to test the private visibility > methods `encodedOutLength` and `decodedOutLength`, which the public `encode` > and `decode` overloaded methods call respectively. This pull request has now been integrated. Changeset: b33096f8 Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/b33096f887108c3d7e1f4e62689c2b10401234fa Stats: 81 lines in 1 file changed: 44 ins; 4 del; 33 mod 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory Reviewed-by: lancea, naoto - PR: https://git.openjdk.org/jdk/pull/19036
RFR: 8331485: Odd Results when Parsing Scientific Notation with Large Exponent
Please review this PR which corrects an edge case bug for
java.text.DecimalFormat that causes incorrect parsing results for strings with
very large exponent values.
When parsing values with large exponents, if the value of the exponent exceeds
`Integer.MAX_VALUE`, the parsed value is equal to 0. If the value of the
exponent exceeds `Long.MAX_VALUE`, the parsed value is equal to the mantissa.
Both results are confusing and incorrect.
For example,
NumberFormat fmt = NumberFormat.getInstance(Locale.US);
fmt.parse(".1E2147483648"); // returns 0.0
fmt.parse(".1E9223372036854775808"); // returns 0.1
// For comparison
Double.parseDouble(".1E2147483648"); // returns Infinity
Double.parseDouble(".1E9223372036854775808"); // returns Infinity
After this change, both parse calls return `Double.POSITIVE_INFINITY` now.
-
Commit messages:
- init
Changes: https://git.openjdk.org/jdk/pull/19075/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=19075=00
Issue: https://bugs.openjdk.org/browse/JDK-8331485
Stats: 165 lines in 2 files changed: 159 ins; 1 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/19075.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19075/head:pull/19075
PR: https://git.openjdk.org/jdk/pull/19075
Re: RFR: 8331582: Incorrect default Console provider comment
On Thu, 2 May 2024 20:54:58 GMT, Naoto Sato wrote: > Fixing a comment in `java.io.Console`, which was a leftover from the fix to > https://bugs.openjdk.org/browse/JDK-8308591 Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/19070#pullrequestreview-2037632558
Re: RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory [v2]
On Wed, 1 May 2024 20:27:19 GMT, Lance Andersen wrote:
>> test/jdk/java/util/Base64/TestEncodingDecodingLength.java line 75:
>>
>>> 73: } catch (InvocationTargetException ex) {
>>> 74: Throwable rootEx = ex.getCause();
>>> 75: assertEquals(OutOfMemoryError.class, rootEx.getClass(),
>>> "00ME should be thrown");
>>
>> Sorry if it is intentional, but I wonder if you meant "OOME" instead of
>> "00ME" here?
>
> Good catch, you sometimes see what you want to see
That was unexpected, wonder how I did that unconsciously...
>> test/jdk/java/util/Base64/TestEncodingDecodingLength.java line 100:
>>
>>> 98: m.invoke(DECODER, src, -LARGE_MEM_SIZE + 1, 1);
>>> 99: } catch (InvocationTargetException ex) {
>>> 100: fail("Decode should neither throw NASE or OOME: " +
>>> ex.getCause());
>>
>> Should we check the cause is either NASE or OOME here?
>
> The original test just validated there was no exception thrown, so I think we
> can just update the message to indicate an unexpected Exception.
>
> I had added this as a comment but must have missed hitting the comment button
Improved the error message to be more general, (as no exception should be
thrown). Comments clarify that the method used to throw NASE and OOME.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19036#discussion_r1586793517
PR Review Comment: https://git.openjdk.org/jdk/pull/19036#discussion_r1586793148
Re: RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory [v3]
> Please review this PR which converts _TestEncodingDecodingLength.java_ into a > whitebox test which allows for testing to be done without memory usage issues. > > Currently, the test requires about ~2.75 `Integer.MAX_VALUE` sized byte > arrays worth of memory. (2 for the initial array allocation, .75 for the > target array in `decode()`). While the `-Xms6g -Xmx8g` options should address > this, there have been intermittent memory issues, as the underlying machine > machine may be running other tests simultaneously. > > By converting this test to a white-box test not only does it get rid of > memory issues, but it also gets rid of the need to decode 2GB of data 3 > times. The change is done using reflection to test the private visibility > methods `encodedOutLength` and `decodedOutLength`, which the public `encode` > and `decode` overloaded methods call respectively. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: reflect further review - Changes: - all: https://git.openjdk.org/jdk/pull/19036/files - new: https://git.openjdk.org/jdk/pull/19036/files/aa25c9a7..eb1f64ce Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19036=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19036=01-02 Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/19036.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19036/head:pull/19036 PR: https://git.openjdk.org/jdk/pull/19036
Re: RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory [v2]
On Wed, 1 May 2024 16:40:31 GMT, Lance Andersen wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reflect review comments
>
> test/jdk/java/util/Base64/TestEncodingDecodingLength.java line 47:
>
>> 45: public class TestEncodingDecodingLength {
>> 46:
>> 47: private static final int size = Integer.MAX_VALUE - 8;
>
> Perhaps consider size-> SIZE and maybe tweak the name and add an additional
> comment to its purpose
Thanks for the review Lance, both comments should be addressed in
https://github.com/openjdk/jdk/pull/19036/commits/aa25c9a72d218401f70bb19bd0e1d911b19b6361
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19036#discussion_r1586643285
Re: RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory [v2]
> Please review this PR which converts _TestEncodingDecodingLength.java_ into a > whitebox test which allows for testing to be done without memory usage issues. > > Currently, the test requires about ~2.75 `Integer.MAX_VALUE` sized byte > arrays worth of memory. (2 for the initial array allocation, .75 for the > target array in `decode()`). While the `-Xms6g -Xmx8g` options should address > this, there have been intermittent memory issues, as the underlying machine > machine may be running other tests simultaneously. > > By converting this test to a white-box test not only does it get rid of > memory issues, but it also gets rid of the need to decode 2GB of data 3 > times. The change is done using reflection to test the private visibility > methods `encodedOutLength` and `decodedOutLength`, which the public `encode` > and `decode` overloaded methods call respectively. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Reflect review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19036/files - new: https://git.openjdk.org/jdk/pull/19036/files/6958ba5a..aa25c9a7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19036=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19036=00-01 Stats: 49 lines in 1 file changed: 20 ins; 7 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/19036.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19036/head:pull/19036 PR: https://git.openjdk.org/jdk/pull/19036
RFR: 8295153: java/util/Base64/TestEncodingDecodingLength.java ran out of memory
Please review this PR which converts _TestEncodingDecodingLength.java_ into a whitebox test which allows for testing to be done without memory usage issues. Currently, the test requires about ~2.75 `Integer.MAX_VALUE` sized byte arrays worth of memory. (2 for the initial array allocation, .75 for the target array in `decode()`). While the `-Xms6g -Xmx8g` options should address this, there have been intermittent memory issues, as the underlying machine machine may be running other tests simultaneously. By converting this test to a white-box test not only does it get rid of memory issues, but it also gets rid of the need to decode 2GB of data 3 times. The change is done using reflection to test the private visibility methods `encodedOutLength` and `decodedOutLength`, which the public `encode` and `decode` overloaded methods call respectively. - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/19036/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19036=00 Issue: https://bugs.openjdk.org/browse/JDK-8295153 Stats: 68 lines in 1 file changed: 32 ins; 9 del; 27 mod Patch: https://git.openjdk.org/jdk/pull/19036.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19036/head:pull/19036 PR: https://git.openjdk.org/jdk/pull/19036
Integrated: 8331207: Misleading example in DateFormat#parse docs
On Fri, 26 Apr 2024 17:47:46 GMT, Justin Lu wrote: > Correct a misleading example in the DateFormat parse documentation. > > Common usage is to use a factory instance. The original example provided > assumes a pattern provided by a COMPAT factory instance. > > The pattern itself should be explicitly provided, since users may be using > CLDR factory instances. This pull request has now been integrated. Changeset: a863ef5d Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/a863ef5d74e9001a143af4638422348ee946c939 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8331207: Misleading example in DateFormat#parse docs Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/18983
Re: RFR: 8331207: Misleading example in DateFormat#parse docs [v2]
> Correct a misleading example in the DateFormat parse documentation. > > Common usage is to use a factory instance. The original example provided > assumes a pattern provided by a COMPAT factory instance. The pattern itself > should be explicitly provided. This is chosen over using a CLDR pattern, as > it uses a NBSP. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: use COMPAT example, CLDR NBSP is problematic - Changes: - all: https://git.openjdk.org/jdk/pull/18983/files - new: https://git.openjdk.org/jdk/pull/18983/files/e5e93258..48978166 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18983=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18983=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18983.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18983/head:pull/18983 PR: https://git.openjdk.org/jdk/pull/18983
RFR: 8331207: Misleading example in DateFormat#parse docs
Correct a misleading example in the DateFormat parse documentation. Common usage is to use a factory instance. The original example provided assumes a pattern provided by a COMPAT factory instance, and does not work with a CLDR instance. The example should be updated to use a pattern provided by a CLDR instance, and also explicitly list this pattern for better clarification. - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/18983/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18983=00 Issue: https://bugs.openjdk.org/browse/JDK-8331207 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18983.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18983/head:pull/18983 PR: https://git.openjdk.org/jdk/pull/18983
Integrated: 8329222: java.text.NumberFormat (and subclasses) spec updates
On Wed, 10 Apr 2024 20:16:50 GMT, Justin Lu wrote: > Please review this PR which is a large spec reformatting for > _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat > and CompactNumberFormat. > > The motivation for this change was the difficulty of readability for these > classes. This PR consists of changes such as better separating the text into > sections with headers, advising to skip the pattern related sections if not > needed, and general wording improvements. > > To help with reviewing I have attached a > [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html) > as well as the built docs: > [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html), > > [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html), > and > [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html); > > I will update the CSR once the wording is finalized. This pull request has now been integrated. Changeset: f60798a3 Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/f60798a30e9a3e0b666fed5384b6ac92a8a283dd Stats: 598 lines in 3 files changed: 234 ins; 204 del; 160 mod 8329222: java.text.NumberFormat (and subclasses) spec updates Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/18731
Re: RFR: 8319990: Update CLDR to Version 45.0
On Mon, 22 Apr 2024 18:44:33 GMT, Naoto Sato wrote: > Upgrading the CLDR to version 45.0. We now have versions specified in the > javadoc (`LocaleServiceProvider`), a corresponding CSR has also been drafted. LGTM; `LocaleServiceProvider` displays correct CLDR version (45), and Italian compact millions is fixed in regression tests - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/18900#pullrequestreview-2015776529
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v4]
> Please review this PR which is a large spec reformatting for > _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat > and CompactNumberFormat. > > The motivation for this change was the difficulty of readability for these > classes. This PR consists of changes such as better separating the text into > sections with headers, advising to skip the pattern related sections if not > needed, and general wording improvements. > > To help with reviewing I have attached a > [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html) > as well as the built docs: > [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html), > > [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html), > and > [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html); > > I will update the CSR once the wording is finalized. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: Make apparent the distinction of cnFmt and dFmt for factory methods. Loosen wording from cast to convert. - Changes: - all: https://git.openjdk.org/jdk/pull/18731/files - new: https://git.openjdk.org/jdk/pull/18731/files/95626d94..33b02f00 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18731=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18731=02-03 Stats: 10 lines in 3 files changed: 0 ins; 1 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18731.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18731/head:pull/18731 PR: https://git.openjdk.org/jdk/pull/18731
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Tue, 16 Apr 2024 21:14:28 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains five commits:
>>
>> - merge master and add setStrict() to nFmt class spec
>> - implement suggestions from dFmt review
>> - implement suggestions from cnFmt review
>> - implement suggestions from nFmt review
>> - init
>
> src/java.base/share/classes/java/text/CompactNumberFormat.java line 78:
>
>> 76: * installed. Thus, to use an instance method defined by {@code
>> CompactNumberFormat},
>> 77: * the {@code NumberFormat} returned by the factory method should first
>> be type
>> 78: * checked before cast to {@code CompactNumberFormat}. If the installed
>> locale-sensitive
>
> Since `CompactNumberFormat` does not provide its own instance methods (i.e.,
> instance methods are exactly the same as the parent NumberFormat), "instance
> methods defined by CompactNF" does not make much sense, which is different
> for `DecimalFormat`.
`CompactNumberFormat` does define its own instance methods such as
`setParseBigDecimal`, `setGroupingSize`, etc, so I think that this wording
should still be included.
> src/java.base/share/classes/java/text/NumberFormat.java line 91:
>
>> 89: * for example, {@link #getIntegerInstance(Locale)}. If the installed
>> locale-sensitive
>> 90: * service implementation does not support the given {@code Locale},
>> {@link Locale#ROOT}
>> 91: * will be used as the fallback {@code Locale}.
>
> The fallback traverses the parent locales, and checks if it is supported or
> not. So it may or may not reach Locale.ROOT. How about "it looks up the
> parent locale chain and uses the one that is supported" or something along
> the lines?
Thanks for the clarification, updated the wording.
> src/java.base/share/classes/java/text/NumberFormat.java line 235:
>
>> 233: * @see ChoiceFormat
>> 234: * @see CompactNumberFormat
>> 235: * @see Locale
>
> Could be removed as the link is now gone
As this is a localized class, and the class description mentions `Locale` in
various places, I feel like `Locale` should still be included as a `see` tag,
(even though I removed the locale link). I am fine either way, what do you
think?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004434
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004370
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1568004906
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v3]
> Please review this PR which is a large spec reformatting for > _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat > and CompactNumberFormat. > > The motivation for this change was the difficulty of readability for these > classes. This PR consists of changes such as better separating the text into > sections with headers, advising to skip the pattern related sections if not > needed, and general wording improvements. > > To help with reviewing I have attached a > [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html) > as well as the built docs: > [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html), > > [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html), > and > [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html); > > I will update the CSR once the wording is finalized. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: implement further feedback - Changes: - all: https://git.openjdk.org/jdk/pull/18731/files - new: https://git.openjdk.org/jdk/pull/18731/files/477970a4..95626d94 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18731=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18731=01-02 Stats: 10 lines in 3 files changed: 2 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18731.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18731/head:pull/18731 PR: https://git.openjdk.org/jdk/pull/18731
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Mon, 15 Apr 2024 18:43:29 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains five commits:
>>
>> - merge master and add setStrict() to nFmt class spec
>> - implement suggestions from dFmt review
>> - implement suggestions from cnFmt review
>> - implement suggestions from nFmt review
>> - init
>
> Here's the last part of the comments for `DecimalFormat`
@naotoj The latest commits should implement your batch of suggestions. I have
also re-built the docs and specdiff with these changes.
> src/java.base/share/classes/java/text/CompactNumberFormat.java line 154:
>
>> 152: * ) in the US locale can be specified as the SimplePattern:
>> {@code "0 Million"}, for the
>> 153: * German locale it can be specified as the PluralPattern:
>> 154: * {@code "{one:0' 'Million other:0' 'Millionen}"}.
>
> I like the German example, however, I think the original explanation for the
> pattern should be preserved (wording might be better though)
I originally removed it because the syntax already covered proper usage. But I
can see how it can be confusing at first to understand. I re-inserted the
original wording, but modified it IMO to be easier to understand.
> src/java.base/share/classes/java/text/CompactNumberFormat.java line 207:
>
>> 205: * {@link java.math.RoundingMode} for formatting. By default, it uses
>> 206: * {@link java.math.RoundingMode#HALF_EVEN RoundingMode.HALF_EVEN}.
>> 207: *
>
> Any reason for this part to move before the `Parsing` section?
I moved it before `Parsing` as it is related to formatting, so its better under
the `Formatting` section.
All the other CompactNumberFormat comments should be implemented as well.
> src/java.base/share/classes/java/text/DecimalFormat.java line 93:
>
>> 91: * for example, {@link #getIntegerInstance(Locale)}. {@link
>> 92: * NumberFormat#getAvailableLocales()} can be used to determine if the
>> desired
>> 93: * locale is supported.
>
> I don't think this section is helpful, or rather, may mislead the users. As
> the original text explains, factory methods in `NumberFormat` do not
> guarantee to return `DecimalFormat` instances by design.
OK, that's a good point. In all 3 of these classes, I adjusted the wording to
recommend using the factory methods for a locale's standard formatting.
However, I made it apparent that the resultant Object can be any subclass of
NumberFormat, and that if users want to cast, (so that they have access to the
subclass instance methods), they should do appropriate type checking first.
All the other DecimalFormat comments you left should be implemented as well.
> src/java.base/share/classes/java/text/NumberFormat.java line 118:
>
>> 116: *
>> 117: * Customizing NumberFormat
>> 118: * {@code NumberFormat} provides API to customize formatting and
>> parsing behavior,
>
> Could make this PR a dependent PR of the leniency PR, and add `setStrict()`
> here as well.
The leniency PR was integrated, I directly added setStrict() to this PR
-
PR Comment: https://git.openjdk.org/jdk/pull/18731#issuecomment-2059552391
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567700054
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567696852
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567696727
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567698329
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
On Tue, 16 Apr 2024 17:09:04 GMT, Justin Lu wrote:
>> Please review this PR which is a large spec reformatting for
>> _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat
>> and CompactNumberFormat.
>>
>> The motivation for this change was the difficulty of readability for these
>> classes. This PR consists of changes such as better separating the text into
>> sections with headers, advising to skip the pattern related sections if not
>> needed, and general wording improvements.
>>
>> To help with reviewing I have attached a
>> [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html)
>> as well as the built docs:
>> [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html),
>>
>> [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html),
>> and
>> [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html);
>>
>> I will update the CSR once the wording is finalized.
>
> Justin Lu has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains five commits:
>
> - merge master and add setStrict() to nFmt class spec
> - implement suggestions from dFmt review
> - implement suggestions from cnFmt review
> - implement suggestions from nFmt review
> - init
src/java.base/share/classes/java/text/NumberFormat.java line 115:
> 113: * attributes or key/type pairs.
> 114: * Below is an example of a "US" locale currency format with
> accounting style,
> 115: * {@code
> NumberFormat.getInstance(Locale.forLanguageTag("en-US-u-cf-account"));}
I replaced the previous Thai digits example with an account currency format
style, as it is probably a more common use case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1567697780
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates [v2]
> Please review this PR which is a large spec reformatting for > _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat > and CompactNumberFormat. > > The motivation for this change was the difficulty of readability for these > classes. This PR consists of changes such as better separating the text into > sections with headers, advising to skip the pattern related sections if not > needed, and general wording improvements. > > To help with reviewing I have attached a > [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html) > as well as the built docs: > [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html), > > [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html), > and > [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html); > > I will update the CSR once the wording is finalized. Justin Lu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - merge master and add setStrict() to nFmt class spec - implement suggestions from dFmt review - implement suggestions from cnFmt review - implement suggestions from nFmt review - init - Changes: https://git.openjdk.org/jdk/pull/18731/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18731=01 Stats: 597 lines in 3 files changed: 233 ins; 204 del; 160 mod Patch: https://git.openjdk.org/jdk/pull/18731.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18731/head:pull/18731 PR: https://git.openjdk.org/jdk/pull/18731
Integrated: 8327640: Allow NumberFormat strict parsing
On Mon, 18 Mar 2024 06:24:54 GMT, Justin Lu wrote:
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
This pull request has now been integrated.
Changeset: 941bee19
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/941bee197ff679e9b8755cad117f5172e3508ef2
Stats: 1603 lines in 12 files changed: 1499 ins; 33 del; 71 mod
8327640: Allow NumberFormat strict parsing
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/18339
Re: RFR: 8329222: java.text.NumberFormat (and subclasses) spec updates
On Fri, 12 Apr 2024 20:58:07 GMT, Naoto Sato wrote:
>> Please review this PR which is a large spec reformatting for
>> _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat
>> and CompactNumberFormat.
>>
>> The motivation for this change was the difficulty of readability for these
>> classes. This PR consists of changes such as better separating the text into
>> sections with headers, advising to skip the pattern related sections if not
>> needed, and general wording improvements.
>>
>> To help with reviewing I have attached a
>> [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html)
>> as well as the built docs:
>> [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html),
>>
>> [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html),
>> and
>> [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html);
>>
>> I will update the CSR once the wording is finalized.
>
> Hi Justin, I went through the `NumberFormat` part. More to follow.
Hi @naotoj , thanks for the detailed review. Working on the suggestions; left a
few responses as well.
> src/java.base/share/classes/java/text/NumberFormat.java line 61:
>
>> 59: * {@code NumberFormat} is the abstract base class for all number
>> 60: * formats. This class provides the interface for formatting and parsing
>> 61: * numbers in a {@link Locale localized} manner. This enables code that
>> can be completely
>
> Linking "localized" to the `Locale` class seems a bit odd to me. A user who
> clicks the link may be stranded.
Makes sense, I'll remove the link.
The intent was to give a reader unfamiliar with i18n a little more background.
But the next sentence expands on locale in that sense so perhaps that's enough.
> src/java.base/share/classes/java/text/NumberFormat.java line 66:
>
>> 64: * digits used. For example, this class could be used within an
>> application to
>> 65: * produce a number in a currency format according to conventions of the
>> locale
>> 66: * a user resides in.
>
> "a user resides in" may not convey any additional value. The user doesn't
> have to be in that locale.
Fair enough, I don't want to give any misconceptions.
The point was to give a general use case example of an app allowing format(s)
tailored to the locale of the user. As I imagine this would be the most common
case, rather than getting a locale separate than that of the user. I'll adjust
the example.
> src/java.base/share/classes/java/text/NumberFormat.java line 91:
>
>> 89: * locale is supported by the installed locale-sensitive service
>> implementation,
>> 90: * either use {@link #getAvailableLocales()} or ensure a factory method
>> call is enclosed
>> 91: * within a try block.
>
> Not sure this is correct. Even if a locale is not "supported" by a locale
> provider, it won't throw an exception, but fallback to, say ROOT, locale.
Good to know, I didn't realize this was the case. This is inherited from the
original wording: `"This will work for the vast majority of locales; just
remember to put it in a try block in case you encounter an unusual one."`
I'll replace it with the actual behavior, that Locale.ROOT is a fallback, if
the particular locale is not supported.
> src/java.base/share/classes/java/text/NumberFormat.java line 135:
>
>> 133: * {@code CompactNumberFormat} depending on the factory method used.
>> For example,
>> 134: * cast to {@code DecimalFormat} to call {@link
>> DecimalFormat#setGroupingSize(int)}
>> 135: * to change the desired digits between grouping separators.
>
> Since there is a possibility that SPIs can return the NumberFormat instances,
> not sure recommending casting is the right thing. If you want to add this, at
> least suggest the type check beforehand.
This is also inherited from the original wording: `"If you want even more
control over the format or parsing, or want to give your users more control,
you can try casting the NumberFormat you get from the factory methods to a
DecimalFormat or CompactNumberFormat depending on the factory method used."`
True that an SPI can return some other NumberFormat instance, but I think that
we still need to emphasize casting to the JDK DecimalFormat or
CompactNumberFormat, otherwise users have no way to access the subclass
instance methods, unless they directly construct the subclass which we don't
recommend.
I'll adjust the wording here (and include type checking)
-
PR Comment: https://git.openjdk.org/jdk/pull/18731#issuecomment-2052601485
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563246011
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563245987
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563246086
PR Review Comment: https://git.openjdk.org/jdk/pull/18731#discussion_r1563246207
Re: RFR: 8327640: Allow NumberFormat strict parsing [v8]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request with a new target base due to a merge or
a rebase. The incremental webrev excludes the unrelated changes brought in by
the merge/rebase. The pull request contains 18 additional commits since the
last revision:
- correct assert statement
- Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing
- Reflect test suggestions
- reflect impl suggestions
- Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing
- improve wording consistency and see tags
- Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing
- revert cleanup changes; (to go into a separate change)
- use directed 'inheritDoc' tag
- update example for lenient parsing
- ... and 8 more: https://git.openjdk.org/jdk/compare/ada2bc7c...6c9c4692
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/eec992e3..6c9c4692
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18339=07
- incr: https://webrevs.openjdk.org/?repo=jdk=18339=06-07
Stats: 4628 lines in 184 files changed: 1334 ins; 1373 del; 1921 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
Re: RFR: 8327640: Allow NumberFormat strict parsing [v6]
On Thu, 11 Apr 2024 01:46:06 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains 14 additional commits >> since the last revision: >> >> - Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing >> - improve wording consistency and see tags >> - Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing >> - revert cleanup changes; (to go into a separate change) >> - use directed 'inheritDoc' tag >> - update example for lenient parsing >> - replace protected field for private fields in subclasses for consistency >> - drop Format implNote as well >> - setStrict and isStrict should be optional in NumberFormat >>- specify and throw UOE as default >>- override and implement in dFmt and cmpctNFmt >>parseStrict should be protected, and utilized by subclasses. The public >> methods should just >>serve as API. >> - Re-work specification wording from Format down to subclasses >> - ... and 4 more: https://git.openjdk.org/jdk/compare/f894b148...aa1c284b > > Looks good overall. Left some minor comments. > As to the tests, good to see those corner cases, but they should have unit > tests for the new methods, i.e, `isStrict()` and `setStrict()`. Also I think > equality/serialization for those methods should be examined. Thank you for the review @naotoj. All of your comments should be reflected in the two previous commits. As for the test suggestions, I added to existing tests if possible, otherwise I created new test files to address your suggestions. The new DecimalFormat equality and serialization test can definitely have other qualities tested (beyond parseStrict), but perhaps that's best for a separate issue. > src/java.base/share/classes/java/text/DecimalFormat.java line 43: > >> 41: import sun.util.locale.provider.LocaleProviderAdapter; >> 42: import sun.util.locale.provider.ResourceBundleBasedAdapter; >> 43: > > Internal packages typically follow public packages. Fixed. (IntelliJ is adamant on having it that way, and even after I previously reverted it, it seems like it slipped through again. Oddly, it never used to do this, need to look at the settings) - PR Comment: https://git.openjdk.org/jdk/pull/18339#issuecomment-2050518798 PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1561677534
Re: RFR: 8327640: Allow NumberFormat strict parsing [v7]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request incrementally with two additional
commits since the last revision:
- Reflect test suggestions
- reflect impl suggestions
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/aa1c284b..eec992e3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18339=06
- incr: https://webrevs.openjdk.org/?repo=jdk=18339=05-06
Stats: 277 lines in 7 files changed: 260 ins; 6 del; 11 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
RFR: JDK-8329222: java.text.NumberFormat (and subclasses) spec updates
Please review this PR which is a large spec reformatting for _java.text.NumberFormat_ and related subclasses, specifically DecimalFormat and CompactNumberFormat. The motivation for this change was the difficulty of readability for these classes. This PR consists of changes such as better separating the text into sections with headers, advising to skip the pattern related sections if not needed, and general wording improvements. To help with reviewing I have attached a [specdiff](https://cr.openjdk.org/~jlu/JDK-8329222/java.base/java/text/package-summary.html) as well as the built docs: [nFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/NumberFormat.html), [dFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/DecimalFormat.html), and [cnFmt](https://cr.openjdk.org/~jlu/api/java.base/java/text/CompactNumberFormat.html); I will update the CSR once the wording is finalized. - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/18731/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18731=00 Issue: https://bugs.openjdk.org/browse/JDK-8329222 Stats: 481 lines in 3 files changed: 166 ins; 152 del; 163 mod Patch: https://git.openjdk.org/jdk/pull/18731.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18731/head:pull/18731 PR: https://git.openjdk.org/jdk/pull/18731
Re: RFR: JDK-8327640: Allow NumberFormat strict parsing [v6]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request with a new target base due to a merge or
a rebase. The incremental webrev excludes the unrelated changes brought in by
the merge/rebase. The pull request contains 14 additional commits since the
last revision:
- Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing
- improve wording consistency and see tags
- Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing
- revert cleanup changes; (to go into a separate change)
- use directed 'inheritDoc' tag
- update example for lenient parsing
- replace protected field for private fields in subclasses for consistency
- drop Format implNote as well
- setStrict and isStrict should be optional in NumberFormat
- specify and throw UOE as default
- override and implement in dFmt and cmpctNFmt
parseStrict should be protected, and utilized by subclasses. The public
methods should just
serve as API.
- Re-work specification wording from Format down to subclasses
- ... and 4 more: https://git.openjdk.org/jdk/compare/5de48d4a...aa1c284b
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/3f2b097a..aa1c284b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18339=05
- incr: https://webrevs.openjdk.org/?repo=jdk=18339=04-05
Stats: 32548 lines in 782 files changed: 14532 ins; 12719 del; 5297 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
Re: RFR: 8329787: Fix typo in CLDRConverter
On Fri, 5 Apr 2024 17:22:36 GMT, Naoto Sato wrote: > Fix a file/class name in the `CLDRConverter` build tool, with some clean-up > for a switch statement. LGTM - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/18662#pullrequestreview-1983814332
Re: RFR: 8329354: java/text/Format/MessageFormat/CompactSubFormats.java fails
On Sat, 30 Mar 2024 02:26:32 GMT, Jie Fu wrote: > Hi all, > > java/text/Format/MessageFormat/CompactSubFormats.java fails in our testing > machines. > I'm not an expert in this area and just guess it can be fixed like this. > Please review it. > > Thanks. > Best regards, > Jie Hi Jie, This is indeed the correct solution to ensure that the DecimalFormat, (specifically, the underlying DecimalFormatSymbols) has the same locale as the MessageFormat subformat. Thank you for the fix, it LGTM - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/18557#pullrequestreview-1970062699
Integrated: JDK-8329118: Run MessageFormat additional subformat pattern tests under en_US locale
On Tue, 26 Mar 2024 20:49:48 GMT, Justin Lu wrote: > Please review this PR which updates two MessageFormat sub format related > tests to be guaranteed to run under the `en_US` locale. > > There exists locale that do not provide distinct instances for separate > styles. For example, the `en_IN` locale provides the same LONG and SHORT > compact number instances. The test data is built to test sub formats under > the assumption that different styles do provide distinct instances. > > As this is the case, these tests should be ran under a locale that does > provide distinct instances for separate styles. This pull request has now been integrated. Changeset: bf93e77e Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/bf93e77e72ccce56685fd10dde83587c703a37b3 Stats: 18 lines in 2 files changed: 6 ins; 0 del; 12 mod 8329118: Run MessageFormat additional subformat pattern tests under en_US locale Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/18498
Re: RFR: 8326627: Document Double/Float.valueOf(String) behavior for numeric strings with non-ASCII digits
On Wed, 27 Mar 2024 19:58:17 GMT, Naoto Sato wrote: > Clarifying the behavior for non-ASCII digits in Double/Float.valueOf(String) > method descriptions. A draft CSR has also been created. New wording LGTM - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/18526#pullrequestreview-1967172723
Integrated: JDK-8327875: ChoiceFormat should advise throwing UnsupportedOperationException for unused methods
On Tue, 26 Mar 2024 20:50:10 GMT, Justin Lu wrote: > Please review this PR which advises ChoiceFormat subclasses throw > `UnsupportedOperationException` for unused inherited methods. > > The CSR covers the proposed wording, and the reasons as to why this is a > specification and not an implementation update. In addition, the > `api/implNote` tags have been moved to the bottom of the class description, > as they are no longer only relevant to the pattern section, (where they were > previously located). This pull request has now been integrated. Changeset: 0cb0b5db Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/0cb0b5db2ac0f9b5a8fe40c5be5f7b12124fe402 Stats: 25 lines in 1 file changed: 15 ins; 10 del; 0 mod 8327875: ChoiceFormat should advise throwing UnsupportedOperationException for unused methods Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/18499
Re: RFR: JDK-8327640: Allow NumberFormat strict parsing [v5]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request with a new target base due to a merge or
a rebase. The incremental webrev excludes the unrelated changes brought in by
the merge/rebase. The pull request contains 13 additional commits since the
last revision:
- improve wording consistency and see tags
- Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing
- revert cleanup changes; (to go into a separate change)
- use directed 'inheritDoc' tag
- update example for lenient parsing
- replace protected field for private fields in subclasses for consistency
- drop Format implNote as well
- setStrict and isStrict should be optional in NumberFormat
- specify and throw UOE as default
- override and implement in dFmt and cmpctNFmt
parseStrict should be protected, and utilized by subclasses. The public
methods should just
serve as API.
- Re-work specification wording from Format down to subclasses
- implement white space suggestions
- ... and 3 more: https://git.openjdk.org/jdk/compare/c8a9b97f...3f2b097a
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/4edb4802..3f2b097a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18339=04
- incr: https://webrevs.openjdk.org/?repo=jdk=18339=03-04
Stats: 349232 lines in 2746 files changed: 15275 ins; 10415 del; 323542 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
Re: RFR: JDK-8329118: Run MessageFormat additional subformat pattern tests under en_US locale [v4]
> Please review this PR which updates two MessageFormat sub format related > tests to be guaranteed to run under the `en_US` locale. > > There exists locale that do not provide distinct instances for separate > styles. For example, the `en_IN` locale provides the same LONG and SHORT > compact number instances. The test data is built to test sub formats under > the assumption that different styles do provide distinct instances. > > As this is the case, these tests should be ran under a locale that does > provide distinct instances for separate styles. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: drop loc field for US constant - Changes: - all: https://git.openjdk.org/jdk/pull/18498/files - new: https://git.openjdk.org/jdk/pull/18498/files/bf99aa92..5e797fe4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18498=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18498=02-03 Stats: 21 lines in 2 files changed: 4 ins; 8 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/18498.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18498/head:pull/18498 PR: https://git.openjdk.org/jdk/pull/18498
Re: RFR: JDK-8329118: Run MessageFormat additional subformat pattern tests under en_US locale [v3]
> Please review this PR which updates two MessageFormat sub format related > tests to be guaranteed to run under the `en_US` locale. > > There exists locale that do not provide distinct instances for separate > styles. For example, the `en_IN` locale provides the same LONG and SHORT > compact number instances. The test data is built to test sub formats under > the assumption that different styles do provide distinct instances. > > As this is the case, these tests should be ran under a locale that does > provide distinct instances for separate styles. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: correct update - Changes: - all: https://git.openjdk.org/jdk/pull/18498/files - new: https://git.openjdk.org/jdk/pull/18498/files/33dc70ab..bf99aa92 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18498=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18498=01-02 Stats: 21 lines in 2 files changed: 1 ins; 0 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/18498.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18498/head:pull/18498 PR: https://git.openjdk.org/jdk/pull/18498
Re: RFR: JDK-8329118: Run MessageFormat additional subformat pattern tests under en_US locale [v3]
On Tue, 26 Mar 2024 22:32:08 GMT, Naoto Sato wrote: >> Thanks for the review and suggestions, PR should be updated. (I presume you >> meant the 2-arg Locale.of() method, not the constructor) > > I meant using `new MessageFormat(String, Locale.US)`, instead of `new > MessageFormat(String)`. Ah, gotchu. Updated with the two arg `MessageFormat` constructor, although I think it would have been fine previously as well just passing the locale in directly to the subformat. - PR Review Comment: https://git.openjdk.org/jdk/pull/18498#discussion_r1540210872
Re: RFR: JDK-8329118: Run MessageFormat additional subformat pattern tests under en_US locale [v2]
> Please review this PR which updates two MessageFormat sub format related > tests to be guaranteed to run under the `en_US` locale. > > There exists locale that do not provide distinct instances for separate > styles. For example, the `en_IN` locale provides the same LONG and SHORT > compact number instances. The test data is built to test sub formats under > the assumption that different styles do provide distinct instances. > > As this is the case, these tests should be ran under a locale that does > provide distinct instances for separate styles. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: implement suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/18498/files - new: https://git.openjdk.org/jdk/pull/18498/files/1ea2cd4e..33dc70ab Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18498=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18498=00-01 Stats: 26 lines in 2 files changed: 9 ins; 4 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/18498.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18498/head:pull/18498 PR: https://git.openjdk.org/jdk/pull/18498
Re: RFR: JDK-8329118: Run MessageFormat additional subformat pattern tests under en_US locale [v2]
On Tue, 26 Mar 2024 21:56:48 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> implement suggestions > > test/jdk/java/text/Format/MessageFormat/CompactSubFormats.java line 29: > >> 27: * @summary Test MessageFormatPattern ability to recognize and produce >> 28: * appropriate FormatType and FormatStyle for >> CompactNumberFormat. >> 29: * @run junit/othervm -Duser.language=en -Duser.country=US >> CompactSubFormats > > I would instantiate MessageFormat explicitly with the `US` locale (using the > 2-arg constructor), instead of implicitly specifying it with the system > property. Thanks for the review and suggestions, PR should be updated. (I presume you meant the 2-arg Locale.of() method, not the constructor) - PR Review Comment: https://git.openjdk.org/jdk/pull/18498#discussion_r1540182774
RFR: JDK-8327875: ChoiceFormat should advise throwing UnsupportedOperationException for unused methods
Please review this PR which advises ChoiceFormat subclasses throw `UnsupportedOperationException` for unused inherited methods. The CSR covers the proposed wording, and the reasons as to why this is a specification and not an implementation update. In addition, the `api/implNote` tags have been moved to the bottom of the class description, as they are no longer only relevant to the pattern section, (where they were previously located). - Commit messages: - remove contract wording - move and clarify api/impl note - init Changes: https://git.openjdk.org/jdk/pull/18499/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18499=00 Issue: https://bugs.openjdk.org/browse/JDK-8327875 Stats: 25 lines in 1 file changed: 15 ins; 10 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18499.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18499/head:pull/18499 PR: https://git.openjdk.org/jdk/pull/18499
RFR: JDK-8329118: Run MessageFormat additional subformat pattern tests under en_US locale
Please review this PR which updates two MessageFormat sub format related tests to be guaranteed to run under the `en_US` locale. There exists locale that do not provide distinct instances for separate styles. For example, the `en_IN` locale provides the same LONG and SHORT compact number instances. The test data is built to test sub formats under the assumption that different styles do provide distinct instances. As this is the case, these tests should be ran under a locale that does provide distinct instances for separate styles. - Commit messages: - run in othervm - init Changes: https://git.openjdk.org/jdk/pull/18498/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18498=00 Issue: https://bugs.openjdk.org/browse/JDK-8329118 Stats: 8 lines in 2 files changed: 4 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/18498.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18498/head:pull/18498 PR: https://git.openjdk.org/jdk/pull/18498
Re: RFR: JDK-8327640: Allow NumberFormat strict parsing [v4]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request incrementally with three additional
commits since the last revision:
- replace protected field for private fields in subclasses for consistency
- drop Format implNote as well
- setStrict and isStrict should be optional in NumberFormat
- specify and throw UOE as default
- override and implement in dFmt and cmpctNFmt
parseStrict should be protected, and utilized by subclasses. The public
methods should just
serve as API.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/c09a34dd..4edb4802
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18339=03
- incr: https://webrevs.openjdk.org/?repo=jdk=18339=02-03
Stats: 148 lines in 4 files changed: 87 ins; 24 del; 37 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
Re: RFR: JDK-8327640: Allow NumberFormat strict parsing [v3]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request incrementally with three additional
commits since the last revision:
- Re-work specification wording from Format down to subclasses
- implement white space suggestions
- remove unsupported methods header, should go in JDK-8327875 instead
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/c3a32500..c09a34dd
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18339=02
- incr: https://webrevs.openjdk.org/?repo=jdk=18339=01-02
Stats: 128 lines in 4 files changed: 21 ins; 64 del; 43 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
Re: RFR: JDK-8327640: Allow NumberFormat strict parsing [v2]
On Tue, 19 Mar 2024 08:56:46 GMT, Andrey Turbanov wrote: >> Justin Lu has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Replace protected field with a public getter -> isStrict(). >> Replace setLenient() with setStrict() to avoid messy inversion of boolean. >> Add a leniency section to NumberFormat. >> Overhaul of NumberFormat class specification to be much more >> organized/readable. > > src/java.base/share/classes/java/text/DecimalFormat.java line 2482: > >> 2480: // process digits or Inf, find decimal position >> 2481: status[STATUS_INFINITE] = false; >> 2482: if (!isExponent && text.regionMatches(position, >> symbols.getInfinity(),0, > > Suggestion: > > if (!isExponent && text.regionMatches(position, > symbols.getInfinity(), 0, Thanks, this and the other suggestions you provided should be fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1534407476
Re: RFR: JDK-8326853 Missing @since tags for Charset related methods added in Java 10 [v8]
On Tue, 19 Mar 2024 17:59:48 GMT, Nizar Benalla wrote: >> Nizar Benalla has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - update the copyright year to 2024 >> - Revert "update the latter years for the Oracle copyrights" >> >>This reverts commit 8bcc364fef8287c4d9aff7c60ed6fc0ea9155f64. > > Thank you justin Hi @nizarbenalla , you can comment `/integrate` whenever you're ready, and we can sponsor the change to have it integrated. - PR Comment: https://git.openjdk.org/jdk/pull/18032#issuecomment-2012982347
Re: RFR: JDK-8326853 Missing @since tags for Charset related methods added in Java 10 [v8]
On Tue, 19 Mar 2024 17:02:36 GMT, Nizar Benalla wrote: >> # Issue >> - JDK-8326853 Incorrect @\since Tags for Charset Related Methods Added in >> JDK 10 >> >> I changed the @\since tags to better accurately show when the methods and >> constructors were introduced. > > Nizar Benalla has updated the pull request incrementally with two additional > commits since the last revision: > > - update the copyright year to 2024 > - Revert "update the latter years for the Oracle copyrights" > >This reverts commit 8bcc364fef8287c4d9aff7c60ed6fc0ea9155f64. Change looks good to me, but you will need someone with reviewer status to sponsor your changes before you can integrate them. BTW, I also added a `noreg-doc` label to your JBS issue, which just means that no tests are needed for this change as it is a documentation only change. - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/18032#pullrequestreview-1946933024
Re: RFR: JDK-8326853 Missing @since tags for Charset related methods added in Java 10 [v7]
On Tue, 19 Mar 2024 17:05:50 GMT, Nizar Benalla wrote: >> src/java.base/share/classes/java/nio/channels/Channels.java line 2: >> >>> 1: /* >>> 2: * Copyright (c) 2000, 2023, 2024, Oracle and/or its affiliates. All >>> rights reserved. >> >> Thanks for updating, but needs a little adjustment. >> >> Rather than adding a third year, the format should be: "original year, >> current year, Oracle ..." >> So in this case -> `2000, 2024,` > > Thanks for the explanation, I have fixed it. > I have a couple other similar PRs, is the policy to update the copyright year > every time a file is changed? whether it's an addition or removal of code? Yes, you should update the copyright year to the current year if you make a change to the file, regardless if the change itself is "significant" or not. Note that there are two formats: `Year1, Year2, Oracle` and `Year, Oracle`. The latter would be used if say you added a new file in 2024. - PR Review Comment: https://git.openjdk.org/jdk/pull/18032#discussion_r1530792055
Re: RFR: JDK-8326853 Missing @since tags for Charset related methods added in Java 10 [v7]
On Tue, 19 Mar 2024 10:54:41 GMT, Nizar Benalla wrote: >> # Issue >> - JDK-8326853 Incorrect @\since Tags for Charset Related Methods Added in >> JDK 10 >> >> I changed the @\since tags to better accurately show when the methods and >> constructors were introduced. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > update the latter years for the Oracle copyrights src/java.base/share/classes/java/nio/channels/Channels.java line 2: > 1: /* > 2: * Copyright (c) 2000, 2023, 2024, Oracle and/or its affiliates. All > rights reserved. Thanks for updating, but needs a little adjustment. Rather than adding a third year, the format should be: "original year, current year, Oracle ..." So in this case -> `2000, 2024,` - PR Review Comment: https://git.openjdk.org/jdk/pull/18032#discussion_r1530694415
Re: RFR: JDK-8326853 Missing @since tags for Charset related methods added in Java 10 [v6]
On Mon, 18 Mar 2024 12:44:46 GMT, Nizar Benalla wrote: >> # Issue >> - JDK-8326853 Incorrect @\since Tags for Charset Related Methods Added in >> JDK 10 >> >> I changed the @\since tags to better accurately show when the methods and >> constructors were introduced. > > Nizar Benalla has updated the pull request incrementally with one additional > commit since the last revision: > > added terminal new line The `@since 10` tags look consistent with the JDK release those methods / constructors were introduced in. Can you update the latter years for the Oracle copyrights only in both these files, thanks. - PR Review: https://git.openjdk.org/jdk/pull/18032#pullrequestreview-1944702560
Re: RFR: JDK-8327640: Allow NumberFormat strict parsing [v2]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
Replace protected field with a public getter -> isStrict().
Replace setLenient() with setStrict() to avoid messy inversion of boolean.
Add a leniency section to NumberFormat.
Overhaul of NumberFormat class specification to be much more
organized/readable.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/8843c56f..c3a32500
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18339=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18339=00-01
Stats: 208 lines in 5 files changed: 77 ins; 54 del; 77 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
RFR: JDK-8327640: Allow NumberFormat strict parsing
Please review this PR and associated
[CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
parsing for NumberFormat.
The concrete subclasses that will utilize this leniency value are
`DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
to be used as an input validation technique for localized formatted numbers.
The default leniency mode will remain lenient, and can only be set to strict
through an intentional `setLenient(boolean)` method call. Leniency operates
differently based off the values returned by `isGroupingUsed()`,
`getGroupingSize()`, and `isParseIntegerOnly()`.
Below is an example of the change, the CSR can be viewed for further detail.
DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
fmt.setLenient(false);
fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
fmt.setGroupingSize(2);
fmt.parse("12,34,567"); // throws ParseException
fmt.setParseIntegerOnly(true)
fmt.parse("12,34.56"); // throws ParseException
fmt.parse("12,34"); // successfully returns the Number 1234
The associated tests are all localized, and the data is converted properly
during runtime.
-
Commit messages:
- init
Changes: https://git.openjdk.org/jdk/pull/18339/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=18339=00
Issue: https://bugs.openjdk.org/browse/JDK-8327640
Stats: 1199 lines in 6 files changed: 1157 ins; 10 del; 32 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
Integrated: JDK-8327631: Update IANA Language Subtag Registry to Version 2024-03-07
On Thu, 7 Mar 2024 23:31:46 GMT, Justin Lu wrote: > Please review this PR which is a trivial update to the IANA sub tag registry > to version 2024-03-07. Tests pass as expected after update. > > Associated announcement -> > https://mm.icann.org/pipermail/ietf-languages-announcements/2024-March/90.html This pull request has now been integrated. Changeset: d5b95a0e Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/d5b95a0ed38b10ed9f51d20255e06eb38fdd8b82 Stats: 80 lines in 2 files changed: 77 ins; 0 del; 3 mod 8327631: Update IANA Language Subtag Registry to Version 2024-03-07 Reviewed-by: naoto, iris - PR: https://git.openjdk.org/jdk/pull/18159
Re: RFR: 8327242: Document supported CLDR versions in the javadoc [v2]
On Tue, 12 Mar 2024 18:50:38 GMT, Naoto Sato wrote: >> Adding a table that maps JDK versions and corresponding CLDR releases as an >> implNote. A draft CSR has also been created. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments LGTM src/java.base/share/classes/java/util/spi/LocaleServiceProvider.java line 144: > 142: * > 143: * The default value for looking up the preferred locale data providers > is "CLDR", > 144: * so specifying "CLDR" is identical to the default behavior. > Applications which Separate to this issue but, rewording as `so specifying only "CLDR"` may improve clarity here. - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/18243#pullrequestreview-1932224881 PR Review Comment: https://git.openjdk.org/jdk/pull/18243#discussion_r1522025588
Re: RFR: 8327705: Remove mention of "applet" from java.text package description
On Fri, 8 Mar 2024 19:54:31 GMT, Naoto Sato wrote: > Removing left over "applet" mention in the package-info doc. Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/18173#pullrequestreview-1925828046
Integrated: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
On Sat, 2 Mar 2024 00: 34: 32 GMT, Justin Lu wrote: > Please review this PR and corresponding CSR which prevents an OutOfMemoryError by restricting the initial maximum fraction digits for an empty pattern DecimalFormat. ZjQcmQRYFpfptBannerStart This Message Is From an External Sender This message came from outside your organization. Report Suspicious ZjQcmQRYFpfptBannerEnd On Sat, 2 Mar 2024 00:34:32 GMT, Justin Lu wrote: > Please review this PR and corresponding CSR which prevents an OutOfMemoryError by restricting the initial maximum fraction digits for an empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, StringBuilder internally doubles capacity attempting to append Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral compatibility changes. This pull request has now been integrated. Changeset: 6efdaf8d Author: Justin Lu URL: https://git.openjdk.org/jdk/commit/6efdaf8ddf2940bcd5f96e114fe05b951ace313b Stats: 259 lines in 3 files changed: 201 ins; 34 del; 24 mod 8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/18094
RFR: JDK-8327631: Update IANA Language Subtag Registry to Version 2024-03-07
Please review this PR which is a trivial update to the IANA sub tag registry to version 2024-03-07. Tests pass as expected after update. Associated announcement -> https://mm.icann.org/pipermail/ietf-languages-announcements/2024-March/90.html - Commit messages: - init Changes: https://git.openjdk.org/jdk/pull/18159/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18159=00 Issue: https://bugs.openjdk.org/browse/JDK-8327631 Stats: 80 lines in 2 files changed: 77 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18159.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18159/head:pull/18159 PR: https://git.openjdk.org/jdk/pull/18159
Re: RFR: 8327167: Add an example for Leap Day in Calendar's doc
On Thu, 7 Mar 2024 19:28:13 GMT, Naoto Sato wrote: > A simple doc update to include a leap day example in the `Calendar` class. Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/18158#pullrequestreview-1923601796
Re: RFR: 8327434: Test java/util/PluggableLocale/TimeZoneNameProviderTest.java timed out
On Wed, 6 Mar 2024 21:32:11 GMT, Naoto Sato wrote: > Fixing timeout failures in the test case. Time zone names that are missing > (chiefly for minor locales) are now calculated at runtime after the fix to > JDK-8174269. This extra calculation time was multiplied in the test case as > it iterated over all locales x timezones, which caused timeouts in some > configurations. Changed the test case to not iterate all locales, but iterate > over relevant ones. Solution makes sense, LGTM - Marked as reviewed by jlu (Committer). PR Review: https://git.openjdk.org/jdk/pull/18143#pullrequestreview-1921004473
Integrated: JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns
On Wed, 14 Feb 2024 22:29:07 GMT, Justin Lu wrote:
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317756)
> which defines the behavior for creating ChoiceFormats with incorrect
> patterns. The wording is added to both the ChoiceFormat constructor and
> ChoiceFormat::applyPattern method.
>
> While ideally the inconsistent behavior itself could be fixed, this behavior
> has been long-standing for 20+ years and the benefit of consistent error
> handling does not outweigh the risk of breaking applications that may be
> relying on the "expected" incorrect behavior.
>
> Examples of the range of behavior, (all examples violate the pattern syntax
> defined in the class description)
>
>
> // no limit -> throws an expected IllegalArgumentException
> var a = new ChoiceFormat("#foo");
> // no limit or relation in the last subPattern -> discards the incorrect
> portion, 'baz' and continues
> var b = new ChoiceFormat("0#foo|1#bar|baz");
> b.format(2); // returns 'bar'
> // no relation or limit -> discards the incorrect portion, 'foo' and continues
> var c = new ChoiceFormat("foo");
> c.format(1); // throws AIOOBE
This pull request has now been integrated.
Changeset: b665fe3a
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/b665fe3ac10f4e85b91737228780b1d50ae81514
Stats: 21 lines in 1 file changed: 13 ins; 0 del; 8 mod
6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/17856
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v4]
> Please review this PR and corresponding CSR which prevents an > OutOfMemoryError by restricting the initial maximum fraction digits for an > empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is > initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, > StringBuilder internally doubles capacity attempting to append > Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral > compatibility changes. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: locale no longer needed, remove unusued imports - Changes: - all: https://git.openjdk.org/jdk/pull/18094/files - new: https://git.openjdk.org/jdk/pull/18094/files/7ee87fc2..e9e86da7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18094=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18094=02-03 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094 PR: https://git.openjdk.org/jdk/pull/18094
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]
On Tue, 5 Mar 2024 19:10:19 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains five additional commits
>> since the last revision:
>>
>> - cleanup/typo
>> - Merge branch 'master' into JDK-8326908-emptyPattern-OOME
>> - implement feedback + improve pattern related tests
>> - minor additions
>> - init
>
> test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 60:
>
>> 58: dFmt.setMinimumFractionDigits(minFrac);
>> 59:
>> 60: String[] patterns = dFmt.toPattern().split("\\.");
>
> Instead of assuming/hardcoding the decimal separator,
> `DecimalFormatSymbols.getInstance(Locale.US).getDecimalSeparator()` may be
> better.
Actually, since `toPattern()` is non-localized, it will always use "." as
specified by DecimalFormat.
I removed the misleading comment `// Use a US dFmt, which uses '.' as the
decimal separator` that indicated we needed to use a DecimalFormatSymbols where
"." was the decimal separator. Can just simply use a DecimalFormat returned by
the default constructor.
> test/jdk/java/text/Format/DecimalFormat/ToPatternTest.java line 110:
>
>> 108: // 8326908: Verify that an empty pattern DecimalFormat does not
>> throw an
>> 109: // OutOfMemoryError when toPattern() is invoked. Behavioral change
>> of
>> 110: // MAXIMUM_INTEGER_DIGITS replaced with DOUBLE_FRACTION_DIGITS for
>> empty
>
> Should we explicitly check `new DecimalFormat("").getMaximumFractionDigits()
> == DOUBLE_FRACTION_DIGITS`?
Thanks for the review, added an explicit check
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513517348
PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1513517336
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v3]
> Please review this PR and corresponding CSR which prevents an > OutOfMemoryError by restricting the initial maximum fraction digits for an > empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is > initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, > StringBuilder internally doubles capacity attempting to append > Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral > compatibility changes. Justin Lu has updated the pull request incrementally with one additional commit since the last revision: reflect review - Changes: - all: https://git.openjdk.org/jdk/pull/18094/files - new: https://git.openjdk.org/jdk/pull/18094/files/7ca56500..7ee87fc2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18094=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18094=01-02 Stats: 20 lines in 2 files changed: 8 ins; 4 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/18094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094 PR: https://git.openjdk.org/jdk/pull/18094
Re: RFR: 8327261: Parsing test for Double/Float succeeds w/o testing all bad cases [v2]
On Tue, 5 Mar 2024 17:26:11 GMT, Naoto Sato wrote: >> Fixing test cases. For bad test cases, only the first case was run, and the >> rest were ignored. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments Marked as reviewed by jlu (Committer). - PR Review: https://git.openjdk.org/jdk/pull/18113#pullrequestreview-1917856118
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
On Sun, 3 Mar 2024 05:00:36 GMT, Guoxiong Li wrote: >> Please review this PR and corresponding CSR which prevents an >> OutOfMemoryError by restricting the initial maximum fraction digits for an >> empty pattern DecimalFormat. >> >> For an empty String pattern DecimalFormat, the maximum fraction digits is >> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, >> StringBuilder internally doubles capacity attempting to append >> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral >> compatibility changes. > >> When toPattern() is invoked, StringBuilder internally doubles capacity >> attempting to append Integer.MAX_VALUE digits until OOME occurs. > > It seems a bug in `toPattern` or `StringBuilder`? May be better to > investigate more about it. Hi @lgxbslgx, For clarification, this is entirely a bug with DecimalFormat, not StringBuilder. An empty String pattern DecimalFormat sets the maximum fraction digits to `Integer.MAX_VALUE`. When toPattern() is invoked, the local StringBuilder will append until an OOME is thrown by the StringBuilder as there is not enough memory, when internally, the buffer is doubled for a value too large. But such an OOME would occur for any large enough value, so the issue lies with DecimalFormat. - PR Comment: https://git.openjdk.org/jdk/pull/18094#issuecomment-1977712428
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]
On Mon, 4 Mar 2024 16:03:55 GMT, Roger Riggs wrote: >> Justin Lu has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains five additional commits >> since the last revision: >> >> - cleanup/typo >> - Merge branch 'master' into JDK-8326908-emptyPattern-OOME >> - implement feedback + improve pattern related tests >> - minor additions >> - init > > src/java.base/share/classes/java/text/DecimalFormat.java line 3717: > >> 3715: // As maxFracDigits are fully displayed unlike maxIntDigits >> 3716: // Prevent OOME by setting to a much more reasonable value. >> 3717: setMaximumFractionDigits(DOUBLE_FRACTION_DIGITS); > > Setting a reasonable default makes sense. > In other control paths, the max fraction digits come from the inputs or are > explicitly set. > > It might be a reasonable related change to use StringBuilder.repeat() instead > of a loop at LIne 3312-3319, where the pattern char(s) are being appended to > the result. Thanks for taking a look. Updated the loop with `repeat` as you suggested, and decided to refactor the rest of the method while I was at it. Additionally, I added some more tests, as it seems that there is a lack of pattern tests for DecimalFormat in general. > In other control paths, the max fraction digits come from the inputs or are > explicitly set. Right, while `Integer.MAX_VALUE` can still be set by other control paths (and thus an OOME if toPattern() is invoked), this is something explicitly done by the user, and thus we decided we would still allow the behavior. - PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1511966791
Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string [v2]
> Please review this PR and corresponding CSR which prevents an > OutOfMemoryError by restricting the initial maximum fraction digits for an > empty pattern DecimalFormat. > > For an empty String pattern DecimalFormat, the maximum fraction digits is > initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, > StringBuilder internally doubles capacity attempting to append > Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral > compatibility changes. Justin Lu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision: - cleanup/typo - Merge branch 'master' into JDK-8326908-emptyPattern-OOME - implement feedback + improve pattern related tests - minor additions - init - Changes: - all: https://git.openjdk.org/jdk/pull/18094/files - new: https://git.openjdk.org/jdk/pull/18094/files/fc8a7ce4..7ca56500 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18094=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18094=00-01 Stats: 5885 lines in 919 files changed: 2483 ins; 1248 del; 2154 mod Patch: https://git.openjdk.org/jdk/pull/18094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094 PR: https://git.openjdk.org/jdk/pull/18094
RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string
Please review this PR and corresponding CSR which prevents an OutOfMemoryError by restricting the initial maximum fraction digits for an empty pattern DecimalFormat. The cause is that the maximum fraction digits is initialized to `Integer.MAX_VALUE` which causes StringBuilder to eventually throw OOME when internally doubling capacity too many times when toPattern is invoked. CSR covers potential behavioral compatibility changes. - Commit messages: - minor additions - init Changes: https://git.openjdk.org/jdk/pull/18094/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18094=00 Issue: https://bugs.openjdk.org/browse/JDK-8326908 Stats: 52 lines in 2 files changed: 51 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18094.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18094/head:pull/18094 PR: https://git.openjdk.org/jdk/pull/18094
Re: RFR: JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns [v6]
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317756)
> which defines the behavior for creating ChoiceFormats with incorrect
> patterns. The wording is added to both the ChoiceFormat constructor and
> ChoiceFormat::applyPattern method.
>
> While ideally the inconsistent behavior itself could be fixed, this behavior
> has been long-standing for 20+ years and the benefit of consistent error
> handling does not outweigh the risk of breaking applications that may be
> relying on the "expected" incorrect behavior.
>
> Examples of the range of behavior, (all examples violate the pattern syntax
> defined in the class description)
>
>
> // no limit -> throws an expected IllegalArgumentException
> var a = new ChoiceFormat("#foo");
> // no limit or relation in the last subPattern -> discards the incorrect
> portion, 'baz' and continues
> var b = new ChoiceFormat("0#foo|1#bar|baz");
> b.format(2); // returns 'bar'
> // no relation or limit -> discards the incorrect portion, 'foo' and continues
> var c = new ChoiceFormat("foo");
> c.format(1); // throws AIOOBE
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
wording: punctuation and use 'constructor'
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17856/files
- new: https://git.openjdk.org/jdk/pull/17856/files/8e5bbe05..aa1071e2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17856=05
- incr: https://webrevs.openjdk.org/?repo=jdk=17856=04-05
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17856.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17856/head:pull/17856
PR: https://git.openjdk.org/jdk/pull/17856
Re: RFR: JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns [v5]
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317756)
> which defines the behavior for creating ChoiceFormats with incorrect
> patterns. The wording is added to both the ChoiceFormat constructor and
> ChoiceFormat::applyPattern method.
>
> While ideally the inconsistent behavior itself could be fixed, this behavior
> has been long-standing for 20+ years and the benefit of consistent error
> handling does not outweigh the risk of breaking applications that may be
> relying on the "expected" incorrect behavior.
>
> Examples of the range of behavior, (all examples violate the pattern syntax
> defined in the class description)
>
>
> // no limit -> throws an expected IllegalArgumentException
> var a = new ChoiceFormat("#foo");
> // no limit or relation in the last subPattern -> discards the incorrect
> portion, 'baz' and continues
> var b = new ChoiceFormat("0#foo|1#bar|baz");
> b.format(2); // returns 'bar'
> // no relation or limit -> discards the incorrect portion, 'foo' and continues
> var c = new ChoiceFormat("foo");
> c.format(1); // throws AIOOBE
Justin Lu has updated the pull request incrementally with two additional
commits since the last revision:
- include ascending order stipulation in constuctor as well
- include apiSpec wording, move to patterns section
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17856/files
- new: https://git.openjdk.org/jdk/pull/17856/files/f1f1dc41..8e5bbe05
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17856=04
- incr: https://webrevs.openjdk.org/?repo=jdk=17856=03-04
Stats: 34 lines in 1 file changed: 10 ins; 14 del; 10 mod
Patch: https://git.openjdk.org/jdk/pull/17856.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17856/head:pull/17856
PR: https://git.openjdk.org/jdk/pull/17856
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v3]
On Mon, 26 Feb 2024 23:37:16 GMT, Naoto Sato wrote:
>> This PR intends to remove the legacy `COMPAT` locale data from the JDK. The
>> `COMPAT` locale data was introduced for applications' migratory purposes
>> transitioning to `CLDR`. It is becoming a technical debt and now is the time
>> to remove it (we've been emitting a warning at JVM startup since JDK21, if
>> the app is using `COMPAT`). A corresponding CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove `GensrcLocaleData.gmk`
Still working on getting a better understanding of all the parts here, but left
some initial comments.
src/java.base/share/classes/java/util/Locale.java line 57:
> 55: import jdk.internal.vm.annotation.Stable;
> 56:
> 57: import sun.security.action.GetPropertyAction;
Although trivial change, not sure if the file needs a copyright year bump; not
exactly sure on the policy here.
src/java.base/share/classes/sun/util/locale/provider/FallbackLocaleProviderAdapter.java
line 86:
> 84: @Override
> 85: // In order to correctly report supported locales
> 86: public BreakIteratorProvider getBreakIteratorProvider() {
More for my understanding but I am curious why FallbackLocaleProviderAdapter
has to override `getBreakIteratorProvider`, but can rely on the
`getCollatorProvider` from JRELocaleProviderAdapter? Also wondering why
"BreakIteratorRules" is fetched when JRELocaleProviderAdapter fetches
"FormatData" if the data is the same COMPAT data.
test/jdk/java/text/Format/DateFormat/Bug6683975.java line 27:
> 25: * @test
> 26: * @bug 6683975 8008577 8174269
> 27: * @summary Make sure that date is formatted correctlyin th locale.
not your change and typo doc nit, but,
Suggestion:
* @summary Make sure that date is formatted correctly in th locale.
test/jdk/java/text/Format/NumberFormat/CurrencyFormat.java line 30:
> 28: * Tests both COMPAT and CLDR data.
> 29: * @modules jdk.localedata
> 30: * @run junit/othervm -Djava.locale.providers=COMPAT CurrencyFormat
The methods `currencySymbolsTest`, `currencySymbolsDataProvider`, and
`getFutureSymbol` can be removed since they are for COMPAT only.
The string array `expectedCOMPATData` can be removed from the data provider
method `currencyFormatDataProvider` as well as `isCompat` variable and usage.
_CurrencySymbols.properties_ can also be deleted since that is what
`currencySymbolsDataProvider` uses to build the data and no other tests rely on
the file.
-
PR Review: https://git.openjdk.org/jdk/pull/17991#pullrequestreview-1901683460
PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1503317351
PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1503375503
PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1503301324
PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1503105517
Re: RFR: 8174269: Remove COMPAT locale data provider from JDK [v2]
On Mon, 26 Feb 2024 21:32:22 GMT, Naoto Sato wrote: >> This PR intends to remove the legacy `COMPAT` locale data from the JDK. The >> `COMPAT` locale data was introduced for applications' migratory purposes >> transitioning to `CLDR`. It is becoming a technical debt and now is the time >> to remove it (we've been emitting a warning at JVM startup since JDK21, if >> the app is using `COMPAT`). A corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Update make/jdk/src/classes/build/tools/cldrconverter/CLDRConverter.java > > Co-authored-by: Andrey Turbanov test/jdk/java/util/Locale/CompatWarning.java line 28: > 26: * @bug 8304982 8174269 > 27: * @summary Check if a warning is logged with COMPAT locale provider > 28: * @run main/othervm -Djava.locale.providers=COMPAT CompatWarning Is it worth adding runs with COMPAT specified with other providers (both before and/or after) for coverage sake. Such as `@run main/othervm -Djava.locale.providers=SPI,COMPAT CompatWarning`. test/jdk/java/util/Locale/ExpectedAdapterTypes.java line 27: > 25: * @test > 26: * @bug 8008577 8138613 8174269 > 27: * @summary Check whether CLDR locale provider adapter is enabled by > default Unless I'm mistaken, there aren't any other dedicated tests to ensuring the FALLBACK adapter is included, it might be worth updating the summary to make it apparent that while the default preference list has CLDR, _FALLBACK_ is always appended, since that is new behavior. test/jdk/java/util/Locale/LocaleProvidersFormat.java line 80: > 78: @Test > 79: @EnabledOnOs(WINDOWS) > 80: @EnabledIfSystemProperty(named = "user.language", matches = "ja") Thanks for fixing, as it is `HOST` the locale value should be based on the machine at startup. Although, I'm wondering how the test passed previously then. - PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1503399036 PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1503401752 PR Review Comment: https://git.openjdk.org/jdk/pull/17991#discussion_r1503397390
Integrated: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern
On Thu, 15 Feb 2024 19:44:42 GMT, Justin Lu wrote:
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
> The fix prevents a limit/format entry from being built when the current
> parsing mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
This pull request has now been integrated.
Changeset: d22d890c
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/d22d890cac3c2c27f89445c65a91909c9cb8f9ad
Stats: 206 lines in 2 files changed: 94 ins; 91 del; 21 mod
8325898: ChoiceFormat returns erroneous result when formatting bad pattern
Reviewed-by: naoto
-
PR: https://git.openjdk.org/jdk/pull/17883
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v6]
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
> The fix prevents a limit/format entry from being built when the current
> parsing mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
Justin Lu has updated the pull request with a new target base due to a merge or
a rebase. The incremental webrev excludes the unrelated changes brought in by
the merge/rebase. The pull request contains 11 additional commits since the
last revision:
- ammend comment on parsing
- Merge branch 'master' into JDK-8325898-ChoiceFormat-trailingPipeBug
- use enhanced switch in stringToNum
- revert enum change
- improve variable name for StringBuilder
- null check at public level. spacing adjustment.
- add additional longer test case
- shorten relational symbol in format comment
- preserve final modifier for next/previousDouble
- reduce visibility of Segment enum
- ... and 1 more: https://git.openjdk.org/jdk/compare/f8638598...05051975
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17883/files
- new: https://git.openjdk.org/jdk/pull/17883/files/de38a988..05051975
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17883=05
- incr: https://webrevs.openjdk.org/?repo=jdk=17883=04-05
Stats: 15147 lines in 427 files changed: 8844 ins; 3288 del; 3015 mod
Patch: https://git.openjdk.org/jdk/pull/17883.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17883/head:pull/17883
PR: https://git.openjdk.org/jdk/pull/17883
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v5]
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
>
> The specific fix for this bug is addressed with the following, on line 305,
>
> if (seg != Segment.FORMAT) {
> // Discard incorrect portion and finish building cFmt
> break;
> }
>
> This prevents a limit/format entry from being built when the current parsing
> mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
use enhanced switch in stringToNum
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17883/files
- new: https://git.openjdk.org/jdk/pull/17883/files/7d792777..de38a988
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17883=04
- incr: https://webrevs.openjdk.org/?repo=jdk=17883=03-04
Stats: 11 lines in 1 file changed: 1 ins; 4 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/17883.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17883/head:pull/17883
PR: https://git.openjdk.org/jdk/pull/17883
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v4]
On Fri, 23 Feb 2024 00:21:57 GMT, Naoto Sato wrote:
>> Justin Lu has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> revert enum change
>
> src/java.base/share/classes/java/text/ChoiceFormat.java line 342:
>
>> 340: } else {
>> 341: num = Double.parseDouble(str);
>> 342: }
>
> Could use enhanced switch?
Yes, much cleaner that way.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17883#discussion_r1500969086
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v4]
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
>
> The specific fix for this bug is addressed with the following, on line 305,
>
> if (seg != Segment.FORMAT) {
> // Discard incorrect portion and finish building cFmt
> break;
> }
>
> This prevents a limit/format entry from being built when the current parsing
> mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
revert enum change
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17883/files
- new: https://git.openjdk.org/jdk/pull/17883/files/c2b8db01..7d792777
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17883=03
- incr: https://webrevs.openjdk.org/?repo=jdk=17883=02-03
Stats: 30 lines in 1 file changed: 0 ins; 13 del; 17 mod
Patch: https://git.openjdk.org/jdk/pull/17883.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17883/head:pull/17883
PR: https://git.openjdk.org/jdk/pull/17883
Integrated: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter
On Wed, 31 Jan 2024 22:24:14 GMT, Justin Lu wrote:
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319344)
> which adds MessageFormat pattern support for the following subformats:
> ListFormat, CompactNumberFormat, and DateTimeFormatter. This change is
> intended to provide pattern support for the more recently added JDK Format
> subclasses, as well as improving java.time formatting within i18n. The draft
> javadoc can be viewed here:
> https://cr.openjdk.org/~jlu/docs/api/java.base/java/text/MessageFormat.html.
> Please see the CSR for more in-depth behavioral changes, as well as
> limitations.
>
> The `FormatTypes`: dtf_date, dtf_time, dtf_datetime, pre-defined
> DateTimeFormatter(s), and list are added.
> The `FormatStyles`: compact_short, compact_long, or, and unit are added.
>
> For example, previously,
>
>
> Object[] args = {LocalDate.of(2023, 11, 16), LocalDate.of(2023, 11, 27)};
> MessageFormat.format("It was {0,date,full}, now it is {1,date,full}", args);
>
>
> would throw `Exception java.lang.IllegalArgumentException: Cannot format
> given Object as a Date`
>
> Now, a user can call
>
>
> MessageFormat.format("It was {0,dtf_date,full}, now it is {1,dtf_date,full}",
> args);
>
>
> which returns "It was Thursday, November 16, 2023, now it is Friday, November
> 17, 2023"
This pull request has now been integrated.
Changeset: 00ffc42c
Author:Justin Lu
URL:
https://git.openjdk.org/jdk/commit/00ffc42cef79d82b2f417c133a48bffec4c7e6b9
Stats: 1258 lines in 7 files changed: 929 ins; 197 del; 132 mod
8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and
DateTimeFormatter
Reviewed-by: naoto, rriggs
-
PR: https://git.openjdk.org/jdk/pull/17663
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v3]
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
>
> This PR includes a lot of cleanup to the applyPattern implementation, the
> specific fix for this bug is addressed with the following, on line 305,
>
> if (seg != Segment.FORMAT) {
> // Discard incorrect portion and finish building cFmt
> break;
> }
>
> This prevents a limit/format entry from being built when the current parsing
> mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
improve variable name for StringBuilder
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17883/files
- new: https://git.openjdk.org/jdk/pull/17883/files/d57fa91a..c2b8db01
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17883=02
- incr: https://webrevs.openjdk.org/?repo=jdk=17883=01-02
Stats: 15 lines in 1 file changed: 0 ins; 0 del; 15 mod
Patch: https://git.openjdk.org/jdk/pull/17883.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17883/head:pull/17883
PR: https://git.openjdk.org/jdk/pull/17883
Re: RFR: JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns [v4]
> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317756)
> which defines the behavior for creating ChoiceFormats with incorrect
> patterns. The wording is added to both the ChoiceFormat constructor and
> ChoiceFormat::applyPattern method.
>
> While ideally the inconsistent behavior itself could be fixed, this behavior
> has been long-standing for 20+ years and the benefit of consistent error
> handling does not outweigh the risk of breaking applications that may be
> relying on the "expected" incorrect behavior.
>
> Examples of the range of behavior, (all examples violate the pattern syntax
> defined in the class description)
>
>
> // no limit -> throws an expected IllegalArgumentException
> var a = new ChoiceFormat("#foo");
> // no limit or relation in the last subPattern -> discards the incorrect
> portion, 'baz' and continues
> var b = new ChoiceFormat("0#foo|1#bar|baz");
> b.format(2); // returns 'bar'
> // no relation or limit -> discards the incorrect portion, 'foo' and continues
> var c = new ChoiceFormat("foo");
> c.format(1); // throws AIOOBE
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
wordsmithing
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17856/files
- new: https://git.openjdk.org/jdk/pull/17856/files/00f8179a..f1f1dc41
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17856=03
- incr: https://webrevs.openjdk.org/?repo=jdk=17856=02-03
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17856.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17856/head:pull/17856
PR: https://git.openjdk.org/jdk/pull/17856
Re: RFR: JDK-8325898: ChoiceFormat returns erroneous result when formatting bad pattern [v2]
> Please review this PR which handles an edge case pattern bug with
> ChoiceFormat.
>
>
> var d = new ChoiceFormat("0#foo|1#bar|baz|") // creates cFmt equivalent to
> "0.0#foo|1.0#bar|1.0#"
> d.format(1) // unexpectedly returns ""
>
>
> Not only does this lead to faulty formatting results, but breaks the
> ChoiceFormat class invariant of duplicate limits.
>
> It would have been preferable if either an exception was thrown when the
> ChoiceFormat was initially created, or the ChoiceFormat formatting 1 returned
> a value of "bar".
>
> For comparison,
>
> var d = new ChoiceFormat("0#foo|1#bar|baz") // creates cFmt equivalent to
> "0.0#foo|1.0#bar"
> d.format(1) // returns "bar"
>
>
> After this change, the first code snippet now returns "bar" when formatting 1
> and discards the "baz" as the second code snippet does.
>
>
> This PR includes a lot of cleanup to the applyPattern implementation, the
> specific fix for this bug is addressed with the following, on line 305,
>
> if (seg != Segment.FORMAT) {
> // Discard incorrect portion and finish building cFmt
> break;
> }
>
> This prevents a limit/format entry from being built when the current parsing
> mode has not yet processed the limit and relation. The previous
> implementation would build an additional limit/format of 1 with an empty
> string. The `break` allows for the discarding of the incorrect portion and
> continuing. Alternatively an exception could have been thrown. For
> consistency with the second code snippet, the former was picked.
>
> https://github.com/openjdk/jdk/pull/17856 may be of interest, which is a
> semi-related specification fix.
Justin Lu has updated the pull request incrementally with one additional commit
since the last revision:
null check at public level. spacing adjustment.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17883/files
- new: https://git.openjdk.org/jdk/pull/17883/files/edfbe7bf..d57fa91a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17883=01
- incr: https://webrevs.openjdk.org/?repo=jdk=17883=00-01
Stats: 5 lines in 1 file changed: 2 ins; 2 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17883.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17883/head:pull/17883
PR: https://git.openjdk.org/jdk/pull/17883
