RFR: 8327640: Allow NumberFormat strict parsing [v6]

Justin Lu jlu at openjdk.org
Thu Apr 11 20:55:14 UTC 2024


On Thu, 11 Apr 2024 01:46:06 GMT, Naoto Sato <naoto at openjdk.org> 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


More information about the core-libs-dev mailing list