8176706: Additional Date-Time Formats #7340

naotoj · 2022-02-03T23:29:54Z

Following the prior discussion [1], here is the PR for the subject enhancement. CSR has also been updated according to the suggestion.

[1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-January/085175.html

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed
Change requires a CSR request to be approved

Issues

JDK-8176706: Additional Date-Time Formats
JDK-8243445: Additional Date-Time Formats (CSR)

Reviewers

Joe Wang (@JoeWang-Java - Reviewer) ⚠️ Review applies to 41408ff
Roger Riggs (@RogerRiggs - Reviewer) ⚠️ Review applies to af3c0d6

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7340/head:pull/7340
$ git checkout pull/7340

Update a local copy of the PR:
$ git checkout pull/7340
$ git pull https://git.openjdk.java.net/jdk pull/7340/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7340

View PR using the GUI difftool:
$ git pr show -t 7340

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7340.diff

…ly thrown on format()

bridgekeeper · 2022-02-03T23:30:24Z

👋 Welcome back naoto! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2022-02-04T00:11:19Z

Webrevs

RogerRiggs · 2022-02-07T21:16:14Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

@@ -5015,10 +5148,16 @@ public int parse(DateTimeParseContext context, CharSequence text, int position)
         * @throws IllegalArgumentException if the formatter cannot be found
         */
        private DateTimeFormatter formatter(Locale locale, Chronology chrono) {
-            String key = chrono.getId() + '|' + locale.toString() + '|' + dateStyle + timeStyle;
+            var dtStyle = dateStyle !=null || timeStyle != null;


Switch to using requestedTemplate == null or != null as the discriminator.

RogerRiggs · 2022-02-07T21:18:25Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

+            validateTemplate();
+        }
+
+        private LocalizedPrinterParser(FormatStyle dateStyle, FormatStyle timeStyle, String requestedTemplate) {
            this.dateStyle = dateStyle;


Drop this constructor; it opens up the possibility of ambiguity between the modes.
Keep only the two constructors with disjoint checking and initialization.

RogerRiggs · 2022-02-07T21:22:12Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

+                String pattern = (dtStyle ?
+                        getLocalizedDateTimePattern(dateStyle, timeStyle, chrono, locale) :
+                        getLocalizedDateTimePattern(requestedTemplate, chrono, locale));
+
                formatter = new DateTimeFormatterBuilder().appendPattern(pattern).toFormatter(locale);
                DateTimeFormatter old = FORMATTER_CACHE.putIfAbsent(key, formatter);


.computeIfAbsent(key, () -> {....}) might be a bit cleaner/clearer here and avoid the race a few lines of code below. (a slight improvement in old code).

Good point. Changed to computeIfAbsent().

RogerRiggs · 2022-02-07T23:16:53Z

src/java.base/share/classes/sun/util/locale/provider/LocaleResources.java

+        // validity check
+        var matcher = VALID_SKELETON_PATTERN.matcher(skeleton);
+        if (!matcher.matches()) {
+            throw new IllegalArgumentException("Requested template is invalid: " + requestedTemplate);


The substitutions are not going to be visible in the exception message.
That might make it harder to identify and fix the cause of the exception. Perhaps the message can make it clear that the matching was done after substitutions and show the 'skeleton'.

JoeWang-Java · 2022-02-08T00:39:04Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

+     */
+    public static String getLocalizedDateTimePattern(String requestedTemplate,
+                                                     Chronology chrono, Locale locale) {
+        Objects.requireNonNull(locale, "requestedTemplate");


Typo, "locale" should have been requestedTemplate.

Good catch!

JoeWang-Java · 2022-02-08T00:58:28Z

src/java.base/share/classes/sun/util/locale/provider/JavaTimeDateTimePatternImpl.java

+    public String getJavaTimeDateTimePattern(String requestedTemplate, String calType, Locale locale) {
+        LocaleProviderAdapter lpa = LocaleProviderAdapter.getResourceBundleBased();
+        // CLDR's 'u'/'U' are not supported in the JDK. Replace them with 'y' instead
+        final var modifiedSkeleton = requestedTemplate.replaceAll("[uU]", "y");


Seems to me requestedTemplate needs to be validated when it gets passed to getLocalizedDateTimePattern, similar as to appendLocalized

This was a left over which should be removed. In fact, CLDR specific pattern symbols should not be accepted as the requested template. Removed the substitutions. As to the validation, it will be performed in the following LocaleResources.getLocalizedPattern() method.

RogerRiggs · 2022-02-10T21:32:13Z

make/jdk/src/classes/build/tools/cldrconverter/Bundle.java

@@ -104,6 +108,10 @@
        "narrow.Eras"
    };

+    // DateFormatItem prefix
+    final static String DATEFORMATITEM_KEY_PREFIX = "DateFormatItem.";
+    final static String DATEFORMATITEM_INPUT_REGIONS_PREFIX = "DateFormatItemInputRegions.";


The canonical order of modifiers is "static final".

RogerRiggs · 2022-02-10T21:50:38Z

src/java.base/share/classes/java/time/format/DateTimeFormatter.java

+     * }
+     * All pattern symbols are optional, and each pattern symbol represents the field it is in,
+     * e.g., 'M' represents the Month field. The number of the pattern symbol letters follows the
+     * same presentation, such as "number" or "text" as in the <a href="#patterns">Patterns for


I would reword as:
* All pattern symbols are optional, and each pattern symbol represents a field,
* for example, 'M' represents the Month field. The number of the pattern symbol letters follows the

RogerRiggs · 2022-02-10T21:53:36Z

src/java.base/share/classes/java/time/format/DateTimeFormatter.java

+     * {@code yMMM} will format the date '2020-06-16' to 'Jun 2020' in the {@link Locale#US US locale}.
+     * <p>
+     * The locale is determined from the formatter. The formatter returned directly by
+     * this method will use the {@link Locale#getDefault() default FORMAT locale}.


Editorial suggestion:

this method uses the {https://github.com/link Locale#getDefault() default FORMAT locale}.

RogerRiggs · 2022-02-10T21:56:14Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

@@ -227,6 +227,41 @@ public static String getLocalizedDateTimePattern(FormatStyle dateStyle, FormatSt
                         CalendarDataUtility.findRegionOverride(locale));
    }

+    /**
+     * Gets the formatting pattern for the requested template for a locale and chronology.


"Returns" is more conventional (than "Gets; though it is consistent within the file)

RogerRiggs · 2022-02-10T21:57:57Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

+     * @param chrono  the Chronology, non-null
+     * @param locale  the locale, non-null
+     * @return the locale and Chronology specific formatting pattern
+     * @throws IllegalArgumentException if {@code requestedTemplate} is invalid


The meaning "Invalid" should be clearly defined; repeating or refering to the template regex.

RogerRiggs · 2022-02-10T22:06:11Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

+     * <ul>
+     * <li>the {@code requestedTemplate} specified to this method
+     * <li>the {@code Locale} of the {@code DateTimeFormatter}
+     * <li>the {@code Chronology}, selecting the best available


Perhaps "best available" should be well defined, or just drop "best".
or ...
"of the {@code DateTimeFormatter} unless overridden"

RogerRiggs · 2022-02-10T22:07:07Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

+     *      "[vz]{0,4}"       // Zone
+     * }
+     * All pattern symbols are optional, and each pattern symbol represents the field it is in,
+     * e.g., 'M' represents the Month field. The number of the pattern symbol letters follows the


Ditto above:
* All pattern symbols are optional, and each pattern symbol represents the field,
* for example, 'M' represents the Month field. The number of the pattern symbol letters follows the

RogerRiggs · 2022-02-10T22:09:56Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

-            return formatter;
+            var useRequestedTemplate = requestedTemplate != null;
+            String key = chrono.getId() + '|' + locale.toString() + '|' +
+                    (useRequestedTemplate ? requestedTemplate : Objects.toString(dateStyle) + timeStyle);


The boolean useRequestedTemplate isn't necessary, replace with requestedTemplate != null.

RogerRiggs · 2022-02-10T22:11:09Z

src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java

@@ -58,4 +59,23 @@ protected JavaTimeDateTimePatternProvider() {
     * @since 9
     */
    public abstract String getJavaTimeDateTimePattern(int timeStyle, int dateStyle, String calType, Locale locale);
+
+    /**
+     * Gets the formatting pattern for the requested template, calendarType, and locale.


"Returns" is more conventional.

RogerRiggs · 2022-02-10T22:12:30Z

src/java.base/share/classes/sun/util/locale/provider/JavaTimeDateTimePatternImpl.java

@@ -63,10 +66,22 @@ public boolean isSupportedLocale(Locale locale) {
    @Override
    public String getJavaTimeDateTimePattern(int timeStyle, int dateStyle, String calType, Locale locale) {
        LocaleResources lr = LocaleProviderAdapter.getResourceBundleBased().getLocaleResources(locale);
-        String pattern = lr.getJavaTimeDateTimePattern(
+        return lr.getJavaTimeDateTimePattern(


Fold the parameters onto a single line.

RogerRiggs · 2022-02-10T22:20:48Z

src/java.base/share/classes/sun/util/locale/provider/LocaleResources.java

+     */
+    private String resolveInputSkeleton(String type) {
+        var regionToSkeletonMap = inputSkeletons.get(type);
+        return regionToSkeletonMap.getOrDefault(locale.getLanguage() + "-" + locale.getCountry(),


This structure computes all the defaults even if the value isn't needed (because the value has to be passed to the getOrDefault method. Perhaps performance isn't an issue.

Indeed, yes. I thought this was simple enough, and as you said, not performance-critical.

magicus · 2022-02-11T08:18:23Z

/label -build

openjdk · 2022-02-11T08:19:16Z

@magicus
The build label was successfully removed.

RogerRiggs

Looks good.

RogerRiggs · 2022-02-11T22:26:03Z

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java

@@ -4986,9 +5101,22 @@ private String getChronologyName(Chronology chrono, Locale locale) {
         * @param timeStyle  the time style to use, may be null
         */
        LocalizedPrinterParser(FormatStyle dateStyle, FormatStyle timeStyle) {


Can the constructors be private.
The combination of package protected and the style of caller doing the validation makes me a bit nervous.
There should not be any callers outside of DateTimeFormatterBuilder.

Changed to private so that it can only be instantiated within DateTimeFormatterBuilder. Same modifications are applied to other DateTimePrinterParser implementations.

RogerRiggs · 2022-02-11T22:28:05Z

src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java

+    /**
+     * Returns the formatting pattern for the requested template, calendarType, and locale.
+     * Concrete implementation of this method will retrieve
+     * a java.time specific pattern from selected Locale Provider.


editorial: "from the selected Locale"...

RogerRiggs · 2022-02-11T22:29:36Z

src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java

+     */
+    public String getJavaTimeDateTimePattern(String requestedTemplate, String calType, Locale locale) {
+        // default implementation throws exception
+        throw new DateTimeException("Formatter is not available for the requested template: " + requestedTemplate);


"Formatting pattern is not available"...

Modified, as well as other minor corrections in the javadoc.

openjdk · 2022-02-16T06:23:19Z

@naotoj This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8176706: Additional Date-Time Formats

Reviewed-by: joehw, rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 123 new commits pushed to the master branch:

0af356b: 8278173: [vectorapi] Add x64 intrinsics for unsigned (zero extended) casts
a24498b: 8281771: Crash in java_lang_invoke_MethodType::print_signature
1aff44b: 8279949: JavaThread::_free_handle_block leaks native memory
394ce5f: 8280825: Modules that "provide" ToolProvider should document the name that can be used
745f7e7: 8281186: runtime/cds/appcds/DumpingWithNoCoops.java fails
1870465: 8281744: x86: Use short jumps in TIG::set_vtos_entry_points
2fe0bf6: 8281748: runtime/logging/RedefineClasses.java failed "assert(addr != __null) failed: invariant"
bc61484: 8280136: Serial: Remove unnecessary use of ExpandHeap_lock
2112a9d: 8246033: bin/print_config.js script uses nashorn jjs tool
1c12b15: 8281741: [testbug] PrintIdealPhaseTest fails with -Xcomp
... and 113 more: https://git.openjdk.java.net/jdk/compare/cda9c3011beeec8df68e78e096132e712255ce1b...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

naotoj · 2022-02-16T16:54:05Z

/integrate

openjdk · 2022-02-16T16:55:08Z

Going to push as commit 9b74c3f.
Since your change was applied there have been 130 commits pushed to the master branch:

0f3d3ac: 8061729: Update java/net tests to eliminate dependency on sun.net.www.MessageHeader and some other internal APIs
395bc14: 8281732: add assert for non-NULL assumption for return of unique_ctrl_out
d8f44aa: 8278067: Make HttpURLConnection default keep alive timeout configurable
7428b37: 8281948: JFR: Parser skips too many bytes for fractional types
d5b4666: 8281829: runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java fails after JDK-8281467
fef5d74: 8281812: x86: Use short jumps in TemplateTable::condy_helper
a86cab8: 8236136: tests which use CompilationMode shouldn't be run w/ TieredStopAtLevel
0af356b: 8278173: [vectorapi] Add x64 intrinsics for unsigned (zero extended) casts
a24498b: 8281771: Crash in java_lang_invoke_MethodType::print_signature
1aff44b: 8279949: JavaThread::_free_handle_block leaks native memory
... and 120 more: https://git.openjdk.java.net/jdk/compare/cda9c3011beeec8df68e78e096132e712255ce1b...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-02-16T16:55:24Z

@naotoj Pushed as commit 9b74c3f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

naotoj added 28 commits January 12, 2022 11:23

8176706: Additional Date-Time Formats

cc5dd4a

temporarily enabling GENERIC

60f56c5

ofLocalizedPattern() -> ofSkeleton()

6fad405

Throws IAE on unavailable skeleton

1f43a54

2021 -> 2022, DateTimeException

c7ffce3

Methods renamed. Spec updated.

aa507f0

Refined the spec

7bc459d

Merge branch 'master' into skeleton

7f7d872

Gracefully throw exception for non-CLDR providers

cf8cc3a

Enabled caching

da88464

Refined substituteInputSkeletons()

d29716d

Merge branch 'master' into skeleton

25b567c

Added regex for the requested template validation

caa283b

Spec updated per review comments

3707d96

regex validation

2532537

Merge branch 'master' into skeleton

4f9c4c4

Added test cases

ab271cc

Spec changed to not using locale as an argument

fcff451

Implemented Missing Skeleton Fields

4beb6e5

Cleanup

0659cfc

Removed exceptions from ofLocalizedPattern() method, as they are lazi…

0252283

…ly thrown on format()

Further cleanups

6839e56

Reverted IllegalArgumentException back

4f68916

Merge branch 'master' into skeleton

cacdfe4

Brought back the part that was mistakenly removed

3fd78fb

Some more tests

146a21d

Tidying up

3bbdb27

Merge branch 'master' into skeleton

2710e6d

naotoj changed the title ~~Skeleton~~ 8176706: Additional Date-Time Formats Feb 3, 2022

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org csr Pull request needs approved CSR before integration labels Feb 3, 2022

Removed trailing space

f9311dc

naotoj marked this pull request as ready for review February 4, 2022 00:05

openjdk bot added the rfr Pull request is ready for review label Feb 4, 2022

RogerRiggs reviewed Feb 7, 2022

View reviewed changes

JoeWang-Java reviewed Feb 8, 2022

View reviewed changes

naotoj added 2 commits February 7, 2022 20:59

Modified per suggestions on the PR

059132f

Fixed LocalizedPrinterParser.toString() to reflect requestedTemplate

41408ff

JoeWang-Java approved these changes Feb 8, 2022

View reviewed changes

RogerRiggs reviewed Feb 10, 2022

View reviewed changes

Addressing review comments

af3c0d6

openjdk bot removed the build build-dev@openjdk.org label Feb 11, 2022

RogerRiggs approved these changes Feb 11, 2022

View reviewed changes

Addressing review comments

399257d

openjdk bot removed the csr Pull request needs approved CSR before integration label Feb 16, 2022

openjdk bot added the ready Pull request is ready to be integrated label Feb 16, 2022

openjdk bot added the integrated Pull request has been integrated label Feb 16, 2022

openjdk bot closed this Feb 16, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 16, 2022

8176706: Additional Date-Time Formats #7340

8176706: Additional Date-Time Formats #7340

Conversation

naotoj commented Feb 3, 2022 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Feb 3, 2022

mlbridge bot commented Feb 4, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naotoj Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicus commented Feb 11, 2022

openjdk bot commented Feb 11, 2022

RogerRiggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 16, 2022

naotoj commented Feb 16, 2022

openjdk bot commented Feb 16, 2022

openjdk bot commented Feb 16, 2022

naotoj commented Feb 3, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 4, 2022 •

edited

Loading

naotoj Feb 8, 2022 •

edited

Loading