On Thu, 11 Apr 2024 01:46:06 GMT, Naoto Sato <na...@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
