<i18n dev> RFR: 8262108: SimpleDateFormat formatting broken for sq_MK Locale [v3]

[Naoto Sato]

On Wed, 14 Apr 2021 17:14:55 GMT, Jaikiran Pai <jpai at 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