Re: RFR: 8333456: CompactNumberFormat integer parsing fails when string has no suffix [v2]

2024-06-05 Thread Naoto Sato 
On Tue, 4 Jun 2024 20:43:09 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.
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   move test to existing test file

LGTM. Thanks for the changes.

-

Marked as reviewed by naoto (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19533#pullrequestreview-2099786378

Re: RFR: 8333456: CompactNumberFormat integer parsing fails when string has no suffix [v2]

2024-06-04 Thread Justin Lu 
> 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&pr=19533&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19533&range=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