On Wed, 14 Apr 2021 17:14:55 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. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Update existing NarrowNamesTest to match expectations > - Naoto's review suggestion Changes requested by naoto (Reviewer). src/java.base/share/classes/sun/util/locale/provider/CalendarNameProviderImpl.java line 193: > 191: } > 192: if (field == AM_PM && !javatime && i > PM) { > 193: // when dealing with calendar fields, > don't set AM_PM field value Make it explicit that this block only serves for `java.util.Calendar`, not `java.time.format.DateTimeFormatter(Builder)`. test/jdk/java/util/Calendar/CalendarDisplayNamesTest.java line 53: > 51: for (final int style : styles) { > 52: final Calendar cal = Calendar.getInstance(); > 53: cal.setLenient(false); Don't think leniency is relevant here. test/jdk/java/util/Calendar/NarrowNamesTest.java line 119: > 117: expectedFieldValues.put("a", Calendar.AM); > 118: expectedFieldValues.put("p", Calendar.PM); > 119: testAM_PM(US, ALL_STYLES, expectedFieldValues); I believe this can be reverted to the original, as the original code compares the exact map (including the length of the map). , i.e., testMap(US, AM_PM, ALL_STYLES, "AM", "PM", RESET_INDEX, "a", "p"); ------------- PR: https://git.openjdk.java.net/jdk/pull/3463
