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