On Fri, 11 Oct 2024 22:23:42 GMT, Justin Lu <[email protected]> wrote:
>> Please review this PR which improves the safety of equality checking for
>> DecimalFormatSymbols. As certain setters did not throw NPE, this allowed for
>> NPE in the equality method. This PR now updates the setters to throw NPE.
>>
>> In addition to the NPEs, there is also a behavioral change that
>> `setInternationalCurrencySymbol` no longer sets currency to null if the
>> `currencyCode` is invalid. Instead, it simply does not update `currency`.
>> This must be done, because we do not want to allow nullable instance
>> variables post `initalizeCurrency`.
>
> Justin Lu has updated the pull request incrementally with one additional
> commit since the last revision:
>
> reflect review
Looks good overall. The test can also check the behavioral change in
`setInternationalCurrencySymbol()`.
src/java.base/share/classes/java/text/DecimalFormatSymbols.java line 767:
> 765: NaN.equals(other.NaN) &&
> 766: // Currency fields are lazy. Init via get call to ensure
> non-null
> 767: getCurrencySymbol().equals(other.getCurrencySymbol()) &&
Probably the same comment refinement can be applied to the location in
`hashCode()`.
test/jdk/java/text/Format/DecimalFormat/SettersShouldThrowNPETest.java line 55:
> 53: .filter(m -> Modifier.isPublic(m.getModifiers()))
> 54: .filter(m -> m.getName().startsWith("set"))
> 55: .filter(m ->
> Stream.of(m.getParameterTypes()).noneMatch(Class::isPrimitive))
Can consolidate into a single filter()?
-------------
PR Review: https://git.openjdk.org/jdk/pull/21315#pullrequestreview-2363659166
PR Review Comment: https://git.openjdk.org/jdk/pull/21315#discussion_r1797484163
PR Review Comment: https://git.openjdk.org/jdk/pull/21315#discussion_r1797485380
