On Tue, 13 Apr 2021 15:03:28 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:
> 
>  - copyright year and @bug id reference in existing test
>  - copyright year on source

Thanks for checking.
`CalendarNameProviderImpl` is an implementation detail, which is shared between 
`j.u.Calendar` and `j.t.f.DateTimeFormatter(Builder)` class, where it provides 
localized names for Calendar fields. For the field `Calendar.AM_PM`, it 
includes not only am/pm translations, but also day periods translations as you 
see. So blindly mapping day period translations to Calendar.AM/PM index is 
incorrect. I'd rather modify the code like:

                            if (field == AM_PM && !javatime && i > PM) {
                                continue;
                            } else {
                                map.put(name, base + i);
                            }

-------------

PR: https://git.openjdk.java.net/jdk/pull/3463

Reply via email to