On Tue, 13 Apr 2021 11:42:41 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review for this proposed fix for > https://bugs.openjdk.java.net/browse/JDK-8262108? > > As noted in a comment in that issue, the bug relates to the return value of > `Calendar.getDisplayNames` for the `Calendar.AM_PM` field. The implementation > has started returning invalid values for the `AM_PM` field after the "day > period" support was added recently in the JDK as part of > https://bugs.openjdk.java.net/browse/JDK-8262108. > > The commit here adds a check in the internal implementation of the display > name handling logic, to special case the `AM_PM` field and properly convert > the display name array indexes (which is an internal detail) to valid values > that represent the `AM_PM` calendar field. > > The commit also has a new jtreg test case `CalendarDisplayNamesTest` > reproduces this issue and verifies the fix. > > After this fix was introduced, I ran the test in > `test/jdk/java/util/Calendar/` and that showed up a failure in an existing > test case `NarrowNamesTest`. Looking at that test case, IMO, the current > testing in that `NarrowNamesTest` is incorrect and is probably what hid this > issue in the first place? To fix this, I have added an additional commit > which updates this test case to properly test the `AM_PM` field values. While we are at this, the `Calendar.getDisplayNames(...)` states this: > .... > @throws IllegalArgumentException * if {@code field} or {@code style} is invalid, * or if this {@code Calendar} is non-lenient and any * of the calendar fields have invalid values If I understand this correctly, if the `Calendar` instance was non-lenient and this method returned invalid values for the `AM_PM` field, it should have thrown an `IllegalArgumentException` (which is a strange exception to throw for an invalid result, but that's a different matter). However, with the current code in upstream master, I saw no such exception being thrown (nor do I see any such validating code in the `Calendar` implementation) when the calendar instance was non-lenient. Is that expected? ------------- PR: https://git.openjdk.java.net/jdk/pull/3463
