Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v5]

[Stephen Colebourne]

On Wed, 11 May 2022 20:04:39 GMT, Naoto Sato  wrote:

>> This is to extend the `Custom ID`s in `java.util.TimeZone` class to support 
>> second-level resolution, enabling round trips with `java.time.ZoneOffset`s. 
>> Corresponding CSR is also being drafted.
>
> Naoto Sato has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains six additional commits since 
> the last revision:
> 
>  - Disallow `UTC` -> `GMT`
>  - Merge branch 'master' into JDK-8285844
>  - Minor fixup
>  - Allowed round-trips for offset-style ZoneIds.
>  - Fixed offsets in milliseconds, added test variations, refined Custom ID 
> definitions
>  - 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all 
> ZoneOffsets and returns GMT unexpected

Marked as reviewed by scolebourne (Author).

-

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

Re: RFR: 8285844: TimeZone.getTimeZone(ZoneOffset) does not work for all ZoneOffsets and returns GMT unexpected [v2]

[Stephen Colebourne]

On Tue, 10 May 2022 17:43:07 GMT, Naoto Sato  wrote:

>> test/jdk/java/util/TimeZone/ZoneOffsetRoundTripTest.java line 43:
>> 
>>> 41: private Object[][] testZoneOffsets() {
>>> 42: return new Object[][] {
>>> 43: {ZoneId.of("Z"), 0},
>> 
>> I know, `ZoneId.of()` should parse this as a `ZoneOffset` and return a 
>> `ZoneOffset` instance, but maybe add also the other string variants with 
>> prefix (`ZoneId.of("UTC+00:00:01")` or `ZoneId.of("GMT+00:00:01")` as data 
>> items. Maybe also use `ZoneOffset.of()` for the plain zones to be explicit.
>
> Added them except "UTC+...", as it is not recognizable as a Custom ID.

Can the test cover `UT` prefix as well? (This is another valid prefix in 
`ZoneId`)

If this PR isn't meant to work with UTC prefix, can a test be added that proves 
it does *not* work.

ie. all these are valid in `ZoneId` - "Z", "UTC", "GMT", "UT", "UTC+01:00", 
"GMT+01:00", "UT+01:00" - and all should have some form of associated test.

-

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

Re: RFR: 8286163: micro-optimize Instant.plusSeconds

[Stephen Colebourne]

On Wed, 4 May 2022 20:27:04 GMT, lennartfricke  wrote:

> Provide micro-benchmark for comparison

Seems reasonable to me. plus(long, long) already has this optimisation.

-

Marked as reviewed by scolebourne (Author).

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

Re: RFR: 8285947: Avoid redundant HashMap.containsKey calls in ZoneName

[Stephen Colebourne]

On Fri, 29 Apr 2022 06:31:22 GMT, Andrey Turbanov  wrote:

> `Map.containsKey` call is sometimes unnecessary, when it's known that Map 
> doesn't contain `null` values.
> Instead we can just use Map.get and compare result with `null`.
> I found one of such place, where Map.containsKey calls could be eliminated - 
> `java.time.format.ZoneName`.
> it gives a bit of performance for `toZid`.
> 
> 
> Benchmark Mode  Cnt   Score   Error  Units
> ZoneNamesBench.useExistingCountry avgt5  10,738 ± 0,065  ns/op
> ZoneNamesBench.useExistingCountryOld  avgt5  13,679 ± 0,089  ns/op
> 
> 
> 
> Benchmark
> 
> 
> @BenchmarkMode(value = Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
> @Measurement(iterations = 5, time = 1, timeUnit = TimeUnit.SECONDS)
> @Fork(1)
> @State(Scope.Benchmark)
> public class ZoneNamesBench {
> 
> @Benchmark
> public String useExistingCountry() {
> return ZoneName.toZid("Africa/Tunis", Locale.ITALY);
> }
> 
> @Benchmark
> public String useExistingCountryOld() {
> return ZoneName.toZidOld("Africa/Tunis", Locale.ITALY);
> }
> }
> 
> 
> 
> public static String toZid(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null) {
> String alias = aliases.get(zid);
> if (alias != null) {
> zid = alias;
> mzone = zidToMzone.get(zid);
> }
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map == null || ((zid = map.get(locale.getCountry())) == 
> null)) {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZid(zid);
> }
> 
> public static String toZid(String zid) {
> return aliases.getOrDefault(zid, zid);
> }
> 
> public static String toZidOld(String zid, Locale locale) {
> String mzone = zidToMzone.get(zid);
> if (mzone == null && aliases.containsKey(zid)) {
> zid = aliases.get(zid);
> mzone = zidToMzone.get(zid);
> }
> if (mzone != null) {
> Map map = mzoneToZidL.get(mzone);
> if (map != null && map.containsKey(locale.getCountry())) {
> zid = map.get(locale.getCountry());
> } else {
> zid = mzoneToZid.get(mzone);
> }
> }
> return toZidOld(zid);
> }
> 
> public static String toZidOld(String zid) {
> if (aliases.containsKey(zid)) {
> return aliases.get(zid);
> }
> return zid;
> }
> 
> 

Logically, this LGTM

-

Marked as reviewed by scolebourne (Author).

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

Re: RFR: 8283842: TestZoneTextPrinterParser.test_roundTripAtOverlap fails: DateTimeParseException

[Stephen Colebourne]

On Wed, 30 Mar 2022 16:46:40 GMT, Naoto Sato  wrote:

> Fixes test failures caused by depending on the default locale. Specifying 
> explicit `Locale.US` will do.

Marked as reviewed by scolebourne (Author).

-

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

Re: RFR: 8283996: Reduce cost of year and month calculations

[Stephen Colebourne]

On Wed, 30 Mar 2022 12:06:39 GMT, Claes Redestad  wrote:

> A few integer divisions and multiplications can be replaced with test + 
> addition, leading to a decent speed-up on java.time microbenchmarks such as 
> `GetYearBench`. Numbers from my local x86 workstation, seeing similar 
> speed-up on aarch64 and other x86 setups.
> 
> Baseline:
> 
> BenchmarkMode  Cnt   Score   
> Error   Units
> GetYearBench.getYearFromMillisZoneOffsetthrpt   15  18.492 ± 
> 0.017  ops/ms
> GetYearBench.getYearFromMillisZoneRegionthrpt   15   6.121 ± 
> 0.135  ops/ms
> GetYearBench.getYearFromMillisZoneRegionNormalized  thrpt   15  18.936 ± 
> 0.012  ops/ms
> GetYearBench.getYearFromMillisZoneRegionUTC thrpt   15   9.283 ± 
> 0.222  ops/ms
> 
> Patched:
> 
> BenchmarkMode  Cnt   Score   
> Error   Units
> GetYearBench.getYearFromMillisZoneOffsetthrpt   15  20.931 ± 
> 0.013  ops/ms
> GetYearBench.getYearFromMillisZoneRegionthrpt   15   6.858 ± 
> 0.167  ops/ms
> GetYearBench.getYearFromMillisZoneRegionNormalized  thrpt   15  20.923 ± 
> 0.017  ops/ms
> GetYearBench.getYearFromMillisZoneRegionUTC thrpt   15  10.028 ± 
> 0.182  ops/ms
> 
> 
> Testing: java.time tests locally, CI tier1+2 ongoing.

I'm happy, based on the assertion that it produces the same result and is 
faster.

-

Marked as reviewed by scolebourne (Author).

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

Re: RFR: 8283681: Improve ZonedDateTime offset handling [v3]

[Stephen Colebourne]

On Fri, 25 Mar 2022 15:16:42 GMT, Claes Redestad  wrote:

>> Richard Startin prompted me to have a look at a case where java.time 
>> underperforms relative to joda time 
>> (https://twitter.com/richardstartin/status/1506975932271190017). 
>> 
>> It seems the java.time test of his suffer from heavy allocations due 
>> ZoneOffset::getRules allocating a new ZoneRules object every time and escape 
>> analysis failing to do the thing in his test. The patch here adds a simple 
>> specialization so that when creating ZonedDateTimes using a ZoneOffset we 
>> don't query the rules at all. This removes the risk of extra allocations and 
>> slightly speeds up ZonedDateTime creation for both ZoneOffset (+14%) and 
>> ZoneRegion (+5%) even when EA works like it should (the case in the here 
>> provided microbenchmark).
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Override ZoneOffset::normalized, cache ZoneOffset::getRules, revert change 
> to add 2nd parameter to ZoneId::getOffset

LGTM

-

Marked as reviewed by scolebourne (Author).

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

Re: RFR: 8283681: Improve ZonedDateTime offset handling [v2]

[Stephen Colebourne]

On Fri, 25 Mar 2022 14:35:46 GMT, Claes Redestad  wrote:

>> Richard Startin prompted me to have a look at a case where java.time 
>> underperforms relative to joda time 
>> (https://twitter.com/richardstartin/status/1506975932271190017). 
>> 
>> It seems the java.time test of his suffer from heavy allocations due 
>> ZoneOffset::getRules allocating a new ZoneRules object every time and escape 
>> analysis failing to do the thing in his test. The patch here adds a simple 
>> specialization so that when creating ZonedDateTimes using a ZoneOffset we 
>> don't query the rules at all. This removes the risk of extra allocations and 
>> slightly speeds up ZonedDateTime creation for both ZoneOffset (+14%) and 
>> ZoneRegion (+5%) even when EA works like it should (the case in the here 
>> provided microbenchmark).
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add nanoOfSecond parameter, make micro less reliant on constants

src/java.base/share/classes/java/time/ZoneOffset.java line 512:

> 510: @Override
> 511: /* package-private */ ZoneOffset getOffset(long epochSecond, int 
> nanoOfSecond) {
> 512: return this;

An alternate approach would be for `ZoneOffset` to cache the instance of 
`ZoneRules` either on construction or first use (racy idiom would be OK). That 
way this issue is solved for the many different places that call 
`zoneId.getRules()`.

-

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

Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v3]

[Stephen Colebourne]

On Fri, 4 Mar 2022 21:17:50 GMT, Jim Laskey  wrote:

>> Several attempts have been made to improve Formatter's numeric performance 
>> by caching the current Locale zero. Such fixes, however, ignore the real 
>> issue, which is the slowness of fetching DecimalFormatSymbols. By directly 
>> caching DecimalFormatSymbols in the Formatter, this enhancement streamlines 
>> the process of accessing Locale DecimalFormatSymbols and specifically 
>> getZeroDigit(). The result is a general improvement in the performance of 
>> numeric formatting. 
>> 
>> 
>> @Benchmark 
>> public void bigDecimalDefaultLocale() { 
>> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f 
>> %1$f", X); 
>> } 
>> 
>> @Benchmark 
>> public void bigDecimalLocaleAlternate() { 
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f 
>> %1$f %1$f", X); 
>> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f 
>> %1$f %1$f", X); 
>> } 
>> 
>> @Benchmark 
>> public void bigDecimalThaiLocale() { 
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f 
>> %1$f %1$f", X); 
>> } 
>> 
>> @Benchmark 
>> public void integerDefaultLocale() { 
>> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d 
>> %1$d", x); 
>> } 
>> 
>> @Benchmark 
>> public void integerLocaleAlternate() { 
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d 
>> %1$d %1$d", x); 
>> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d 
>> %1$d %1$d", x); 
>> } 
>> 
>> @Benchmark 
>> public void integerThaiLocale() { 
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d 
>> %1$d %1$d", x); 
>> } 
>> 
>> 
>> Before: 
>> Benchmark Mode Cnt Score Error Units 
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s 
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s 
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s 
>> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s 
>> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s 
>> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s 
>> 
>> After: 
>> Benchmark Mode Cnt Score Error Units 
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s 
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s 
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s 
>> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s 
>> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s 
>> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Too many 'the'

Just to note that there is also some caching in `DecimalStyle`. It might be 
possible to reuse `DecimalStyle` here (if it was extended to support grouping 
separators).

-

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

Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v3]

[Stephen Colebourne]

On Fri, 4 Mar 2022 23:05:56 GMT, Naoto Sato  wrote:

>> Supporting `IsoFields` temporal fields in chronologies that are similar to 
>> ISO chronology. Corresponding CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addresses review comments

src/java.base/share/classes/java/time/chrono/Chronology.java line 794:

> 792:  * @since 19
> 793:  */
> 794: default boolean supportsIsoFields() {

I'm not a fan of this name, as it is inconsistent with the rest of JSR310 API, 
which uses an `is` prefix for booleans. I suggested `isIsoLike` because the key 
question is whether the chronology is "like" ISO. I would also be OK with 
`isBasedOnIso`, `isDerivedFromIso`, `isIsoBased` or something similar. Another 
risk here is limiting the method to refer only to `IsoFields`. While that is 
the use case here, it isn't the case that the only fields that might be 
affected are in `IsoFields`. Third parties may have  their own fields that are 
suitable for use with an ISO-like chronology.

-

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

Re: RFR: 8282081: java.time.DateTimeFormatter: wrong definition of symbol F

[Stephen Colebourne]

On Mon, 28 Feb 2022 23:17:57 GMT, Naoto Sato  wrote:

> Fixing the definition and implementation of the pattern symbol `F`. Although 
> it is an incompatible change, I believe it is worth the fix. For that, a CSR 
> has been drafted.

Although there is incompatibility, I believe a fix is the correct choice. The 
issue arose because of the poor description of the field in LDML.

-

Marked as reviewed by scolebourne (Author).

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

Re: RFR: 8282131: java.time.ZoneId should be a sealed abstract class

[Stephen Colebourne]

On Fri, 25 Feb 2022 19:02:55 GMT, Naoto Sato  wrote:

> Refactoring `java.time.ZoneId` class to be a sealed class. A CSR has also 
> been drafted.

Marked as reviewed by scolebourne (Author).

-

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

Re: Additional Date-Time Formats

[Stephen Colebourne]

On Thu, 27 Jan 2022 at 19:23, Naoto Sato  wrote:
> Now come to think of it, I came up with the draft based on `ofPattern()`
> methods. One of them is a overload method that takes a Locale argument.
> Why is it so?

There is a case for the Locale on the static method (as a convenience,
and to remind callers that the locale matters). But there is no case
for it on the builder.

> > The spec Javadoc doesn't explain what repeating the pattern letter
> > actually does/means. Is "M" the same as ""?
>
> That depends on the locale and the availability of the formats. For
> example, 'M' translates into these patterns in each locale with
> Gregorian calendar:
>
> https://unicode-org.github.io/cldr-staging/charts/40/by_type/date_&_time.gregorian.html#959cbb42bb2962f

As things stand, the Javadoc isn't a standalone spec. I don't know how
much that matters, but I think there should be some indication in
Javadoc as to why the letter should be repeated.

Stephen

Re: Additional Date-Time Formats

[Stephen Colebourne]

Hi,
This would be a useful addition. Some comments:

There is no need for the method overload that takes Locale. The other
similar methods all operate using the locale of the formatter, and
have this Javadoc:

 * The locale is determined from the formatter. The formatter
returned directly by
 * this method will use the {@link Locale#getDefault() default
FORMAT locale}.
 * The locale can be controlled using {@link
DateTimeFormatter#withLocale(Locale) withLocale(Locale)}
 * on the result of this method.

And `appendLocalizedPattern` should not take a Locale either. Again ,
it would use the locale of the formatter instance, calculating the
actual pattern on-demand when the formatter is run.

The spec Javadoc doesn't explain what repeating the pattern letter
actually does/means. Is "M" the same as ""?

thanks
Stephen






On Thu, 20 Jan 2022 at 21:47, Naoto Sato  wrote:
>
> Hello,
>
> I am proposing a couple of new factory methods in
> java.time.format.DateTimeFormatter that produce flexible localized
> date/time formats, other than the existing pre-defined
> (FULL/LONG/MEDIUM/SHORT) styles. For example, if the user wants a year
> and month only string, such as the title for the calendar, currently
> they would have to use DateTimeFormatter.ofPattern() with explicit
> pattern argument, such as "MMM y". This is problematic because it is
> correct in locales such as US, but not correct in other locales.
>
> So, I propose methods that are parallel to ofPattern(), which take
> pattern template. This is based on the CLDR's skeleton:
> https://www.unicode.org/reports/tr35/tr35-dates.html#availableFormats_appendItems
>
> Detailed design can be found in the draft CSR:
> https://bugs.openjdk.java.net/browse/JDK-8243445
>
> Comments are welcome.
>
> Naoto
>

Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v4]

[Stephen Colebourne]

On Mon, 29 Nov 2021 17:41:34 GMT, Naoto Sato  wrote:

>> This fix intends to honor the type (std/dst/generic) of parsed zone name for 
>> selecting the offset at the overlap. Corresponding CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Refined wording. Removed `LENIENT` parser. Added tests for CI.

Marked as reviewed by scolebourne (Author).

-

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

Re: RFR: 8177819: DateTimeFormatterBuilder zone parsing should recognise DST [v3]

[Stephen Colebourne]

On Wed, 24 Nov 2021 23:25:22 GMT, Naoto Sato  wrote:

>> This fix intends to honor the type (std/dst/generic) of parsed zone name for 
>> selecting the offset at the overlap. Corresponding CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Replaced integer literals.
>  - Refined wording #2

src/java.base/share/classes/java/time/format/DateTimeFormatter.java line 508:

> 506:  * of {@link ChronoLocalDateTime#atZone(ZoneId)}. If the {@code ZoneId} 
> was
> 507:  * parsed from the zone name that does not indicate daylight saving 
> time, then
> 508:  * the standard offset will be used at the local time-line overlap.

As written, I would read it as being the generic zone name that gets altered, 
rather than a zone name that explicitly indicates "winter" time. maybe:

"If the {@code ZoneId} was parsed from a zone name that indicates whether 
daylight saving time in in operation or not, then that fact will be used to 
select the correct offset at the local time-line overlap."

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
4906:

> 4904: private static class LENIENT extends CI {
> 4905: 
> 4906: private LENIENT(String k, String v, int t, PrefixTree 
> child) {

Is class `LENIENT` actually in use?

-

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

Re: RFR: 8276775: ZonedDateTime/OffsetDateTime.toString return invalid ISO-8601 for years <= 1893

[Stephen Colebourne]

On Tue, 9 Nov 2021 22:29:12 GMT, Naoto Sato  wrote:

> Simple doc clarification where the `toString()` output only conforms to ISO 
> 8601 if the seconds in the offset are zero.

Marked as reviewed by scolebourne (Author).

-

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

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v10]

[Stephen Colebourne]

On Wed, 3 Nov 2021 22:55:23 GMT, Claes Redestad  wrote:

>> Claes Redestad has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Remove accidentally committed experimental @Stable (no effect on micros)
>
> Thanks, Naoto!

@cl4es For `DateTimeFormatterBuilder ` spec, I propose changing the existing 
wording slightly

If the field value in the date-time to be printed is invalid it
cannot be printed and an exception will be thrown.

to 

If the field value in the date-time to be printed is outside the
range of valid values then an exception will be thrown.

-

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

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v6]

[Stephen Colebourne]

On Wed, 3 Nov 2021 13:14:42 GMT, Claes Redestad  wrote:

>> Prompted by a request from Volkan Yazıcı I took a look at why the java.time 
>> formatters are less efficient for some common patterns than custom 
>> formatters in apache-commons and log4j. This patch reduces the gap, without 
>> having looked at the third party implementations. 
>> 
>> When printing times:
>> - Avoid turning integral values into `String`s before appending them to the 
>> buffer 
>> - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of 
>> `BigDecimal`
>> 
>> This means a speed-up and reduction in allocations when formatting almost 
>> any date or time pattern, and especially so when including sub-second parts 
>> (`S-S`).
>> 
>> Much of the remaining overhead can be traced to the need to create a 
>> `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` 
>> internally. We could likely also win performance by specializing some common 
>> patterns.
>> 
>> Testing: tier1-3
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add test verifying OOB values throw exception

Thanks for adding the new tests, and finding that fraction formatting is more 
constrained than I thought it was.

I think there is a case for a spec update in `DateTimeFormatterBuilder` to 
clarify the constraint on the input value (at this point, that seems better 
than changing the behaviour of fraction formatting). As things stand, the 
exception that is thrown is not described by the spec. (The spec change could 
be part of this PR or a separate one).

-

Marked as reviewed by scolebourne (Author).

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

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v3]

[Stephen Colebourne]

On Tue, 2 Nov 2021 11:03:02 GMT, Claes Redestad  wrote:

>> Prompted by a request from Volkan Yazıcı I took a look at why the java.time 
>> formatters are less efficient for some common patterns than custom 
>> formatters in apache-commons and log4j. This patch reduces the gap, without 
>> having looked at the third party implementations. 
>> 
>> When printing times:
>> - Avoid turning integral values into `String`s before appending them to the 
>> buffer 
>> - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of 
>> `BigDecimal`
>> 
>> This means a speed-up and reduction in allocations when formatting almost 
>> any date or time pattern, and especially so when including sub-second parts 
>> (`S-S`).
>> 
>> Much of the remaining overhead can be traced to the need to create a 
>> `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` 
>> internally. We could likely also win performance by specializing some common 
>> patterns.
>> 
>> Testing: tier1-3
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add fallback for values outside the allowable range

Changes requested by scolebourne (Author).

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
3158:

> 3156: 
> 3157: // only instantiated and used if there's ever a value outside 
> the allowed range
> 3158: private FractionPrinterParser fallback;

This class has to be safe in a multi-threaded environment. I'm not convinced it 
is safe right now, as the usage doesn't follow the standard racy single check 
idiom. At a minimum, there needs to be a comment explaining the thread-safety 
issues.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
3266:

> 3264: if (!field.range().isValidIntValue(value)) {
> 3265: if (fallback == null) {
> 3266: fallback = new FractionPrinterParser(field, 
> minWidth, maxWidth, decimalPoint, subsequentWidth);

It would be nice to see a test case cover this.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
3290:

> 3288: range.checkValidValue(value, field);
> 3289: BigDecimal minBD = BigDecimal.valueOf(range.getMinimum());
> 3290: BigDecimal rangeBD = 
> BigDecimal.valueOf(range.getMaximum()).subtract(minBD).add(BigDecimal.ONE);

I wouldn't be surprised if there is a way to replace the use of `BigDecimal` 
with calculations using `long`.  Fundamentally, calculations like 15/60 -> 0.25 
are not hard, but it depends on whether the exact results can be matched across 
a wide range of possible inputs.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
3544:

> 3542: BigDecimal valueBD = 
> BigDecimal.valueOf(value).subtract(minBD);
> 3543: BigDecimal fraction = valueBD.divide(rangeBD, 9, 
> RoundingMode.FLOOR);
> 3544: // stripTrailingZeros bug

I believe this bug was fixed a while back, so this code could be simplified.

-

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

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter [v2]

[Stephen Colebourne]

On Mon, 1 Nov 2021 22:35:58 GMT, Claes Redestad  wrote:

>> The commentary on this line could probably be improved, but this is in a 
>> private printer-parser that will only be used for NANO_OF_SECOND and not any 
>> arbitrary `TemporalField` (see line 704), thus I fail to see how this 
>> assumption can fail (since NANO_OF_SECOND specifies a value range from 0 to 
>> 999,999,999).
>> 
>> I considered writing a more generic integral-fraction printer parser that 
>> would optimize for any value-range that fits in an int, but seeing how 
>> NANO_OF_SECOND is likely the only one used in practice and with a high 
>> demand for better efficiency I opted to specialize for it more directly.
>
> I see what you're saying that an arbitrary `Temporal` could define its own 
> fields with its own ranges, but I would consider it a design bug if such an 
> implementation at a whim redefines the value ranges of well-defined constants 
> such as `ChronoField.NANO_OF_SECOND` or `HOUR_OF_DAY`. I'd expect such a 
> `Temporal` would have to define its own enumeration of allowed 
> `TemporalField`s.

That isn't the design model however. The design model for the formatter is a 
`Map` like view of field to value. Any value may be associated with any field - 
that is exactly what `Temporal` offers. 
[`TempralAccessor.getLong()`](https://download.java.net/java/early_access/loom/docs/api/java.base/java/time/temporal/TemporalAccessor.html#getLong(java.time.temporal.TemporalField))
 is very explicit about this.

As indicated above, the positive part is that an hour-of-day of 26 can be 
printed by a user-written `WrappingLocalTime` class. The downside is the 
inability to make optimizing assumptions as per this code.

FWIW, I had originally intended to write dedicated private formatters where the 
pattern and type to be formatted are known, such as `LocalDate` and the ISO 
pattern.

-

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

Re: RFR: 8276220: Reduce excessive allocations in DateTimeFormatter

[Stephen Colebourne]

On Mon, 1 Nov 2021 13:04:20 GMT, Claes Redestad  wrote:

> Prompted by a request from Volkan Yazıcı I took a look at why the java.time 
> formatters are less efficient for some common patterns than custom formatters 
> in apache-commons and log4j. This patch reduces the gap, without having 
> looked at the third party implementations. 
> 
> When printing times:
> - Avoid turning integral values into `String`s before appending them to the 
> buffer 
> - Specialize `appendFraction` for `NANO_OF_SECOND` to avoid use of 
> `BigDecimal`
> 
> This means a speed-up and reduction in allocations when formatting almost any 
> date or time pattern, and especially so when including sub-second parts 
> (`S-S`).
> 
> Much of the remaining overhead can be traced to the need to create a 
> `DateTimePrintContext` and adjusting `Instant`s into a `ZonedDateTime` 
> internally. We could likely also win performance by specializing some common 
> patterns.
> 
> Testing: tier1-3

Changes requested by scolebourne (Author).

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
3269:

> 3267: return false;
> 3268: }
> 3269: int val = value.intValue(); // NANO_OF_SECOND must fit in 
> an int and can't be negative

Unfortunately, this is not a valid assumption, and it affects the logic of the 
optimization more generally (methods where non-negative is assumed).

Basically, NANO_OF_SECOND (and all other fields in the formatter) can have any 
`long` value. Despite immediate appearances, the value is not limited to 0 to 
999,999,999. This is because you are allowed to create an implementation of 
`Temporal` that returns values outside that range. No such class exists in the 
JDK, but such a class would be perfectly legal. As a related example, it would 
be perfectly value to write a time class that ran from 03:00 to 26:59 each day, 
thus HOUROF_DAY cannot be assumed by the formatter to be between 0 and 23.

-

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

Re: Discussion: easier Stream closing

[Stephen Colebourne]

On Sun, 26 Sept 2021 at 10:29, Tagir Valeev  wrote:
> List list =
> Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
>
> What do you think?

I fully support this. We have our own utility to do this kind of
thing, as bugs with `Files` are common:
https://github.com/OpenGamma/Strata/blob/main/modules/collect/src/main/java/com/opengamma/strata/collect/io/SafeFiles.java

Stephen

Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v5]

[Stephen Colebourne]

On Wed, 8 Sep 2021 00:36:29 GMT, Naoto Sato  wrote:

>> Please review the fix to the issue. Avoiding overflow by not calling 
>> nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference 
>> in nano unit.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added comments for the test.

`toEpochMilli()` overflows at large/small values. Thus any attempt to calculate 
the difference between two very large instants would fail.

-

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

Re: RFR: 8273369: Computing micros between two instants unexpectedly overflows for some cases [v5]

[Stephen Colebourne]

On Wed, 8 Sep 2021 00:36:29 GMT, Naoto Sato  wrote:

>> Please review the fix to the issue. Avoiding overflow by not calling 
>> nanosUntil() directly, which will overflow beyond Long.MAX_VALUE difference 
>> in nano unit.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added comments for the test.

This change looks fine, but I think you also need a `millisUntil` private 
method to fix the identical overflow problem with millis (which might as well 
be fixed now).

-

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

Re: RFR: 8272473: Parsing epoch seconds at a DST transition with a non-UTC parser is wrong

[Stephen Colebourne]

On Mon, 23 Aug 2021 16:42:03 GMT, Naoto Sato  wrote:

> Please review the fix to the subject issue. When instant seconds and zone 
> co-exist in parsed data, instant seconds was not resolved correctly from them.

LGTM

-

Marked as reviewed by scolebourne (Author).

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

Re: RFR: 8171382: java.time.Duration missing isPositive method

[Stephen Colebourne]

On Fri, 23 Jul 2021 17:27:27 GMT, Naoto Sato  wrote:

> Please review this PR to introduce `java.time.Duration.isPositive()` method. 
> A CSR is also drafted.

Marked as reviewed by scolebourne (Author).

src/java.base/share/classes/java/time/Duration.java line 596:

> 594:  */
> 595: public boolean isPositive() {
> 596: return (seconds | nanos) > 0;

I had to think whether this logic is correct, but I believe it is because 
`nanos` is 32 bits and positive so won't impact the negative bit of `seconds`.

-

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

Re: RFR: 8266901: Clarify the method description of Duration.toDaysPart()

[Stephen Colebourne]

On Mon, 21 Jun 2021 18:22:26 GMT, Naoto Sato  wrote:

> Please review this doc clarification fix to `toDaysPart()` method. CSR will 
> also be filed accordingly.

Marked as reviewed by scolebourne (Author).

-

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

Re: RFR: 8268469: Update java.time to use switch expressions [v3]

[Stephen Colebourne]

On Thu, 17 Jun 2021 13:56:00 GMT, Daniel Fuchs  wrote:

>> It is your codebase, not mine, so it is up to you. Aligning things by column 
>> is generally frowned on in most style guides because it handles refactoring 
>> poorly, resulting in lots of needless change (or people forgetting to 
>> realign things - I had to deal with a rogue aligned Javadoc signature today 
>> in a PR where exactly that had happened). I also don't see how you write a 
>> style guide rule for when these should be aligned and when they should not.
>> 
>> Anyway, this PR isn't really the right place for this discussion - I'm not 
>> blocking the PR on this basis (and I'm not an official Reviewer anyway).
>
> I have a slight preference for the aligned version in the cases where the 
> lines are short and the number of spaces used for padding is not 
> unreasonable. I find the code gains in readability in these cases. In the 
> case where the lines are long - I agree with Stephen that the alignment 
> doesn't bring much readability - and sometime it may even be detrimental as 
> it makes long lines longer. Stephen, would that be an acceptable compromise?

Its certainly a reasonable position to take (ie. if thats what OpenJDK wants to 
do, its fine by me).
I'm more interested to see how you would write such a thing down in the coding 
standards that doesn't make the standard worthless.

-

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

Re: RFR: 8268469: Update java.time to use switch expressions [v3]

[Stephen Colebourne]

On Wed, 16 Jun 2021 11:11:30 GMT, Chris Hegarty  wrote:

>> src/java.base/share/classes/java/time/Month.java line 480:
>> 
>>> 478: int leap = leapYear ? 1 : 0;
>>> 479: return switch (this) {
>>> 480: case JANUARY   -> 1;
>> 
>> Unnecessary alignment
>
> The vertical alignment improves readability in these short-line cases. 
> Removing the spaces before the arrows will make it a little harder to discern 
> the difference between the cases.

It is your codebase, not mine, so it is up to you. Aligning things by column is 
generally frowned on in most style guides because it handles refactoring 
poorly, resulting in lots of needless change (or people forgetting to realign 
things - I had to deal with a rogue aligned Javadoc signature today in a PR 
where exactly that had happened). I also don't see how you write a style guide 
rule for when these should be aligned and when they should not.

Anyway, this PR isn't really the right place for this discussion - I'm not 
blocking the PR on this basis (and I'm not an official Reviewer anyway).

-

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

Re: RFR: 8268469: Update java.time to use switch expressions [v3]

[Stephen Colebourne]

On Wed, 16 Jun 2021 10:57:07 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.time` 
>> packages to make use of the switch expressions?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains four additional 
> commits since the last revision:
> 
>  - 8268469: Removed excessive spacing; corrected misplaced comments
>  - Merge remote-tracking branch 'origin/master' into JDK-8268469
>  - Merge remote-tracking branch 'origin/master' into JDK-8268469
>  - 8268469: Update java.time to use switch expressions

src/java.base/share/classes/java/time/LocalDate.java line 607:

> 605: if (field instanceof ChronoField chronoField) {
> 606: if (chronoField.isDateBased()) {
> 607: int n = switch (chronoField) {

This logic is harder to read than before, and it requires the CPU to perform an 
extra branch on `(n == -1)`. It should just be a `return switch ...`

src/java.base/share/classes/java/time/LocalDateTime.java line 1181:

> 1179: if (unit instanceof ChronoUnit chronoUnit) {
> 1180: return switch (chronoUnit) {
> 1181: case NANOS -> plusNanos(amountToAdd);

Still has unnecessary alignment

There are lots of cases in the PR where `->` is still aligned. I'd like to see 
all of them unaligned.

src/java.base/share/classes/java/time/Month.java line 480:

> 478: int leap = leapYear ? 1 : 0;
> 479: return switch (this) {
> 480: case JANUARY   -> 1;

Unnecessary alignment

-

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

Re: Some possible enhancements for java.time.Duration?

[Stephen Colebourne]

On Tue, 15 Jun 2021 at 15:16,  wrote:
> Comparison
> --
>   boolean isGreaterThan(Duration duration) / isLongerThan(..)
>   boolean isSmallerThan(Duration duration) / isShorterThan(..)
>
> English is not my primary language so I don't know which of these
> aliases would be better. Given that classes such as Instant and
> OffsetDateTime also have comparison methods (isAfter, isBefore), I
> think it would be useful if Duration had easy comparison methods. Of
> course we have compareTo(..) but I always get confused what it actually
> means when I get a positive or negative number. :)

Maybe. Classes like OffsetDateTime have such methods because equals()
and compareTo() have different definitions. My gut feeling is that
this should not be done until Valhalla project has decided what if
anything it will do with the need for operator overloading on value
types.


> More comparison
> ---
>   static Duration max(Duration d1, Duration d2)
>   static Duration min(Duration d1, Duration d2)

I don't see the point of this. If such methods are to be added, they
should be generic, taking any Comparable


> Disallowing negative value
> --
> Okay, this one is a bit more farfetched, but a method which would be
> useful for my use case is to have an alternative of between() which is
> never negative and just truncates to zero if it happens to be negative.
>
> I'm measuring a duration between timestamps which come from different
> systems which should be in sync with their NTP servers (and each
> other), but there may still be some time dilation which could lead to
> weird results making it look like the effect happened before the cause.

I understand the use case, but it is too marginal for Duration IMO.

thanks
Stephen

Re: RFR: 8268469: Update java.time to use switch expressions

[Stephen Colebourne]

On Wed, 9 Jun 2021 15:41:59 GMT, Patrick Concannon  
wrote:

> Hi,
> 
> Could someone please review my code for updating the code in the `java.time` 
> packages to make use of the switch expressions?
> 
> Kind regards,
> Patrick

My biggest comment is the spaces used to align, which I strongly object to. I 
agree with @dfuch comments too.

src/java.base/share/classes/java/time/LocalDate.java line 608:

> 606: if (chronoField.isDateBased()) {
> 607: return switch (chronoField) {
> 608: case DAY_OF_MONTH  -> ValueRange.of(1, 
> lengthOfMonth());

For the record, I hate code that is aligned like this. It makes refactoring 
much more noisy, as the author has to readjust all the associated lines. I also 
don't believe it is within the spirit of Java's traditional coding guidelines.

src/java.base/share/classes/java/time/chrono/ThaiBuddhistDate.java line 331:

> 329: yield with(isoDate.withYear(nvalue - 
> YEARS_DIFFERENCE));
> 330: }
> 331: case ERA -> with(isoDate.withYear((1 - 
> getProlepticYear()) - YEARS_DIFFERENCE));

`checkValidIntValue` performs validation, so removing it has changed the 
behavoiur

src/java.base/share/classes/java/time/format/SignStyle.java line 129:

> 127: // valid if negative or (positive and lenient)
> 128: case 0  -> !positive || !strict;// NORMAL
> 129: case 1,   4 -> true; // ALWAYS, EXCEEDS_PAD

Extra space before `4`

-

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

Integrated: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

This pull request has now been integrated.

Changeset: 6c838c56
Author:    Stephen Colebourne 
Committer: Roger Riggs 
URL:   
https://git.openjdk.java.net/jdk/commit/6c838c568c2c99145fd0ae8059de2b2865b65863
Stats: 623 lines in 6 files changed: 536 ins; 64 del; 23 mod

8266846: Add java.time.InstantSource

Reviewed-by: rriggs, naoto, darcy

-

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

Re: RFR: 8266846: Add java.time.InstantSource [v6]

[Stephen Colebourne]

> 8266846: Add java.time.InstantSource

Stephen Colebourne has updated the pull request incrementally with one 
additional commit since the last revision:

  8266846: Add java.time.InstantSource

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4016/files
  - new: https://git.openjdk.java.net/jdk/pull/4016/files/f01c5cdd..d564956c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=04-05

  Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4016.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016

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

Re: RFR: 8266846: Add java.time.InstantSource [v5]

[Stephen Colebourne]

> 8266846: Add java.time.InstantSource

Stephen Colebourne has updated the pull request incrementally with one 
additional commit since the last revision:

  8266846: Add java.time.InstantSource

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4016/files
  - new: https://git.openjdk.java.net/jdk/pull/4016/files/c7d9076b..f01c5cdd

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=03-04

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4016.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016

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

Re: Proposal for new interface: TimeSource

[Stephen Colebourne]

Hi all,
Is there anything I need to do to progress the CSR and/or PR?
thanks
Stephen

On Thu, 13 May 2021 at 22:05, Stephen Colebourne  wrote:
>
> On Wed, 12 May 2021 at 18:41, Roger Riggs  wrote:
> > Will you be posting a PR for the implementation?
> > It is frequently helpful to evaluate the CSR and the implementation
> > concurrently.
>
> PR: https://github.com/openjdk/jdk/pull/4016
> Issue: https://bugs.openjdk.java.net/browse/JDK-8266846
> CSR: https://bugs.openjdk.java.net/browse/JDK-8266847
>
> The PR takes a middle ground approach to the implementation.
>
> It is not practical to remove the existing package-scoped Clock
> implementation classes (SystemClock, TickClock, FixedClock,
> OffsetClock) despite the fact that these would be better expressed as
> classes that only implement `InstantSource`. However, given that
> "system" is the 99%+ use case, I do believe it is worth adding a
> dedicated `SystemInstantSource` class, as per the PR.
>
> To achieve this I moved the actual logic using
> `VM.getNanoAdjustment()` into a static method which can then be called
> directly from three places - Instant.now(), SystemClock and
> SystemInstantSource. Previously, every instance of SystemClock
> performed the VM/offset calculations separately. The new logic
> performs them once based on a single shared static variable. I have no
> reason to believe this changes the memory model or performance, but I
> must flag it up for reviewers.
>
> When initially discussing the proposal, I planned to add a new static
> method `Clock.of(InstantSource, ZoneId)`. When implementing the change
> I found that the method was adding no value as the instance method
> `InstantSource.withZone(ZoneId)` achieves the same outcome, so I
> omitted it.
>
> The Mac test failure appears to be unconnected to the change.
>
> Thanks for any and all reviews!
> Stephen

Re: RFR: 8266846: Add java.time.InstantSource [v3]

[Stephen Colebourne]

On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne  
wrote:

>> 8266846: Add java.time.InstantSource
>
> Stephen Colebourne has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8266846: Add java.time.InstantSource

I've made the obvious changes to fix the offset reflection. However it does now 
mean that the offset is being altered for a singleton where previously it would 
have only affected Clock.systemUtc(). Is the test class spun up in its own JVM 
process? Just want to make sure that manipulating the singleton clock won't 
impact other tests.

-

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

Re: RFR: 8266846: Add java.time.InstantSource [v4]

[Stephen Colebourne]

> 8266846: Add java.time.InstantSource

Stephen Colebourne has updated the pull request incrementally with one 
additional commit since the last revision:

  8266846: Add java.time.InstantSource

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4016/files
  - new: https://git.openjdk.java.net/jdk/pull/4016/files/425e01a8..c7d9076b

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=02-03

  Stats: 6 lines in 2 files changed: 2 ins; 1 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4016.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016

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

Re: RFR: 8266846: Add java.time.InstantSource [v2]

[Stephen Colebourne]

On Tue, 18 May 2021 21:04:18 GMT, Naoto Sato  wrote:

>> Stephen Colebourne has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8266846: Add java.time.InstantSource
>
> test/jdk/java/time/test/java/time/TestInstantSource.java line 86:
> 
>> 84: assertEquals(test.instant(), instant);
>> 85: assertSame(test.equals(InstantSource..fixed(instant));
>> 86: assertEquals(test.hashCode(), 
>> InstantSource..fixed(instant).hashCode());
> 
> Not sure this would compile.

I was expecting the GitHub Action to pickup coding and test issues (since my 
dev setup can't compile or run tests). But it seems it doesn't. do that. The 
latest commit should at least compile as I copied the test class to another IDE 
location, but I have no way of knowing if the tests pass.

-

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

Re: RFR: 8266846: Add java.time.InstantSource [v3]

[Stephen Colebourne]

On Sun, 16 May 2021 07:39:21 GMT, Peter Levart  wrote:

>> Stephen Colebourne has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/Clock.java line 487:
> 
>> 485: // it more unlikely to hit the 1ns in the future condition.
>> 486: localOffset = System.currentTimeMillis()/1000 - 1024;
>> 487: 
> 
> Is it possible that after a fresh localOffset is retrieved, the thread is 
> preempted and when it is scheduled again after a pause, the 
> getNanoTimeAdjustment below returns -1 ? Would it help if instead of throwing 
> exception, there was an infinite retry loop?

This isn't my logic - it is existing code that has been moved. I'm not a fan of 
infinite retry loops as the can hang the system. But I'm happy to change it to 
work that way if there is a consensus to do so.

-

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

Re: RFR: 8266846: Add java.time.InstantSource [v3]

[Stephen Colebourne]

> 8266846: Add java.time.InstantSource

Stephen Colebourne has updated the pull request incrementally with one 
additional commit since the last revision:

  8266846: Add java.time.InstantSource

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4016/files
  - new: https://git.openjdk.java.net/jdk/pull/4016/files/7fa9d0ec..425e01a8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=01-02

  Stats: 12 lines in 3 files changed: 1 ins; 3 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4016.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016

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

Re: RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

FWIW, if there is a need to access `VM.getNanoTimeAdjustment(localOffset)` then 
that should be a separate issue. I don't personally think it would be approved 
given Valhalla is coming.

-

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

Re: RFR: 8266846: Add java.time.InstantSource [v2]

[Stephen Colebourne]

On Mon, 17 May 2021 14:30:20 GMT, Roger Riggs  wrote:

>> Stephen Colebourne has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/Clock.java line 128:
> 
>> 126:  * Implementations should implement {@code Serializable} wherever 
>> possible and must
>> 127:  * document whether or not they do support serialization.
>> 128:  *
> 
> The ImplSpec needs to say how it is implemented.
> The 'implements InstantSource' can not mandate any particular implementation. 
> Its just an interface the real behavior comes from its implementations.  In 
> this case Clock.  Referring to the static methods of InstantSource behavior 
> may be sufficient because that behavior is concrete.

There are plenty of examples of interfaces in `java.time` and elsewhere that 
apply restrictions to implementations. Nevertheless, for simplicity and 
expediency I have reinstated the `implSpec` on `Clock`

-

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

Re: RFR: 8266846: Add java.time.InstantSource [v2]

[Stephen Colebourne]

> 8266846: Add java.time.InstantSource

Stephen Colebourne has updated the pull request incrementally with one 
additional commit since the last revision:

  8266846: Add java.time.InstantSource

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4016/files
  - new: https://git.openjdk.java.net/jdk/pull/4016/files/befc7621..7fa9d0ec

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4016=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4016=00-01

  Stats: 38 lines in 2 files changed: 31 ins; 0 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4016.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016

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

Re: RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Mon, 17 May 2021 14:13:06 GMT, Roger Riggs  wrote:

>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/InstantSource.java line 165:
> 
>> 163:  */
>> 164: static InstantSource fixed(Instant fixedInstant) {
>> 165: return Clock.fixed(fixedInstant, ZoneOffset.UTC);
> 
> Instant might also implement InstantSource, returning itself.
> This fixed method would be unnecessary because an Instant is itself a 
> InstantSource.

While I can see the appeal, I don't think this would be a good choice. Doing so 
would add three methods to `Instant` that are of no value. `millis()` would 
duplicate `toEpochMilli()`, `withZone(zone)` would duplicate `atZone(zone)` and 
`instant()` would return `this`. Moreover it doesn't respect the rationale of 
the interface, which is explictly to return the current instant, not any random 
instant.

Effectively, this proposal is a variation of Scala's implicit conversions which 
were rejected during the development of JSR-310 as being more likely to cause 
bugs than help developers,

-

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

Re: RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Mon, 17 May 2021 14:04:24 GMT, Roger Riggs  wrote:

>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/InstantSource.java line 36:
> 
>> 34:  * Instances of this interface are used to find the current instant.
>> 35:  * As such, an {@code InstantSource} can be used instead of {@link 
>> System#currentTimeMillis()}.
>> 36:  * 
> 
> The word 'current' is likely to misleading here.  The specification of an 
> interface does not have any context in which to describe what the instant 
> represents or what it is relative to.
> Given the intended use cases, it is definitely not always related to 
> System.currentTimeMillis() 
> which is bound to the running system.
> i think the best you could say is that it returns an instant provided by the 
> source as does the 'instance()' method.

This is the definition used by `Clock` since Java 8. It is also the purpose of 
the interface. ie. this isn't an interface for providing instants in general, 
but for providing the _current instant_. I can clarify wrt the meaning of 
"current", but I don't see how that word can be avoided.

-

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

Re: RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Thu, 13 May 2021 20:52:33 GMT, Stephen Colebourne  
wrote:

> 8266846: Add java.time.InstantSource

It would good to get confirmation in the review from @dholmes-ora or Mark 
Kralj-Taylor that my changes to the precise system clock are correct. 
Specifically, I moved the code from instance to static. I don't believe this 
changes the thread-safety, but it does need double checking (there was no 
discussion in the original thread concerning instance vs static).

Original thread: 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066670.html

-

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

Re: RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Sat, 15 May 2021 21:32:54 GMT, Ralph Goers 
 wrote:

>> 8266846: Add java.time.InstantSource
>
> Can you possibly find a way that this can be used in a garbage free manner?  
> Providing a MutableInstant interface that allows the Instant object to be 
> provided and have its values set by a currentInstant(MutableInstant instant) 
> method would solve the problem nicely for Log4j - or any other app that is 
> trying to avoid allocating new objects. I believe this also aligns with what 
> [Michael Hixson](https://github.com/michaelhixson) is asking for.
> 
> An alternative would be for java.time to maintain a pool of Instants that 
> apps like Log4j can somehow control, but that would seem far more complicated.

@rgoers Michael's request has been handled, which was a simple change to the 
wording. What you need cannot be accomplished because `Instant` has a 
restricted design due to the Valhalla project.

A `MutableInstant` class can have its own `now()` method that does whatever is 
necessary to initialize it, so there is no need to refer to it here.

-

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

Re: RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Sat, 15 May 2021 20:53:28 GMT, Istvan Neuwirth 
 wrote:

>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/InstantSource.java line 93:
> 
>> 91:  * @since 17
>> 92:  */
>> 93: public interface InstantSource {
> 
> Should not we add `@FunctionalInterface`? I can easily imagine this interface 
> being used in tests where we can define the `InstantSource` with lambdas.

`@FunctionalInterface` isn't required for use by lambdas. I wasn't initially 
convinced using it here was the right choice.

-

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

Re: RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

On Fri, 14 May 2021 07:19:03 GMT, Anthony Vanelverdinghe 
 wrote:

>> 8266846: Add java.time.InstantSource
>
> src/java.base/share/classes/java/time/Clock.java line 513:
> 
>> 511:  * {@link System#currentTimeMillis()} or equivalent.
>> 512:  */
>> 513: static final class SystemInstantSource implements InstantSource, 
>> Serializable {
> 
> Is it possible to declare this as an enum? As doing so would simplify its 
> implementation.

My feeling is that in this case it is better to keep control of the kind of 
class being used.

-

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

RFR: 8266846: Add java.time.InstantSource

[Stephen Colebourne]

8266846: Add java.time.InstantSource

-

Commit messages:
 - 8266846: Add java.time.InstantSource
 - 8266846: Add java.time.InstantSource

Changes: https://git.openjdk.java.net/jdk/pull/4016/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4016=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8266846
  Stats: 615 lines in 4 files changed: 519 ins; 83 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4016.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4016/head:pull/4016

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

Re: Proposal for new interface: TimeSource

[Stephen Colebourne]

On Wed, 12 May 2021 at 18:41, Roger Riggs  wrote:
> Will you be posting a PR for the implementation?
> It is frequently helpful to evaluate the CSR and the implementation
> concurrently.

PR: https://github.com/openjdk/jdk/pull/4016
Issue: https://bugs.openjdk.java.net/browse/JDK-8266846
CSR: https://bugs.openjdk.java.net/browse/JDK-8266847

The PR takes a middle ground approach to the implementation.

It is not practical to remove the existing package-scoped Clock
implementation classes (SystemClock, TickClock, FixedClock,
OffsetClock) despite the fact that these would be better expressed as
classes that only implement `InstantSource`. However, given that
"system" is the 99%+ use case, I do believe it is worth adding a
dedicated `SystemInstantSource` class, as per the PR.

To achieve this I moved the actual logic using
`VM.getNanoAdjustment()` into a static method which can then be called
directly from three places - Instant.now(), SystemClock and
SystemInstantSource. Previously, every instance of SystemClock
performed the VM/offset calculations separately. The new logic
performs them once based on a single shared static variable. I have no
reason to believe this changes the memory model or performance, but I
must flag it up for reviewers.

When initially discussing the proposal, I planned to add a new static
method `Clock.of(InstantSource, ZoneId)`. When implementing the change
I found that the method was adding no value as the instance method
`InstantSource.withZone(ZoneId)` achieves the same outcome, so I
omitted it.

The Mac test failure appears to be unconnected to the change.

Thanks for any and all reviews!
Stephen

Proposal for new interface: TimeSource

[Stephen Colebourne]

This is a proposal to add a new interface to java.time.*

  public interface TimeSource {
public static TimeSource system() { ... }
public abstract Instant instant();
public default long millis() {
  return instant().toEpochMilli();
}
public default Clock withZone(ZoneId zoneId) {
  return Clock.of(this, zoneId);
   }
  }

The existing `Clock` abstract class would be altered to implement the interface.
A new static method `Clock.of(TimeSource, ZoneId)` would be added.
No changes are needed to any other part of the API.
(Could add `Instant.now(TimeSource)`, but it isn't entirely necessary)

Callers would pass around and use `TimeSource` directly, for example
by dependency injection.


Why add this interface?
I've seen various indications that there is a desire for an interface
representing a supplier of `Instant`. Specifically this desire is
driven by the "unnecessary" time zone that `java.time.Clock` contains.
Put simply, if the only thing you want is an `Instant`, then `Clock`
isn't the right API because it forces you to think about time zones.

A key thing that this interface allows is the separation of the OS
Clock from the time-zone (which should generally be linked to user
localization). A good architecture would pass `TimeSource` around the
system and combine it with a time-zone held in a `UserContext` to get
a `Clock`. The current design of java.time.* does not enable that
because the `TimeSource` concept does not exist. Developers either
have to write their own interface, or use `Clock` and ignore the time
zone.

A `Supplier` obviously performs similar functionality, but it
lacks discoverability and understandability. Plus, injecting
generified interfaces tends to be painful.

Downsides?
None really, other than a new type in the JDK that probably should
have been in Java 8.


See this ThreeTen-Extra discussion
- https://github.com/ThreeTen/threeten-extra/issues/150

Joda-Time has a `MillisProvider` that is similar:
- 
https://www.joda.org/joda-time/apidocs/org/joda/time/DateTimeUtils.MillisProvider.html

Time4J has a `TimeSource`:
- 
https://github.com/MenoData/Time4J/blob/master/base/src/main/java/net/time4j/base/TimeSource.java

Spring has a `DateTimeContext` that separates the user localization as
per the `UserContext` described above:
- 
https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/format/datetime/standard/DateTimeContext.html

There is a similar concept `TimeSource` in `sun.net.httpserver`

There may be a case to name the interface `InstantSource`, however
`TimeSource` is a fairly widely understood name for this concept.


There is the potential to extend the interface with something similar
to Kotlin's `TimeMark` that would allow access to the monotonic clock,
suitable for measurement of durations without any leap second effects:
- https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.time/-time-mark/

Feedback?

Stephen

Re: Collection::getAny discussion

[Stephen Colebourne]

On Fri, 30 Apr 2021 at 19:50, Stuart Marks  wrote:
> You're asking for something that's somewhat different, which you called the 
> "find
> the first element when there is only one" problem. Here, there's a 
> precondition that
> the collection have a single element. (It's not clear to me what should 
> happen if
> the collection has zero or more than one element.)

I think any get() or getAny() method on Collection is semantically
equivalent to iterator.next(). I'm not sure there is another viable
option.

>   * onlyElement -- if source has 1 element, returns it; throws NSEE if empty, 
> IAE if > 1
>   * toOptional -- if source has 0 or 1 elements, returns an Optional; 
> otherwise throws

These should be added to the JDK. They are useful.

Stephen

Re: New Collections interface - Sized

[Stephen Colebourne]

On Fri, 23 Apr 2021 at 23:07, Brian Goetz  wrote:
> >> Is there a compelling example of where this would be used by clients?
> > Here are some examples:
> > https://stackoverflow.com/questions/10988634/java-global-isempty-method
> Without passing judgment on the sort of dynamically typed programs that
> need a method like this

The other example is better as it benefits from declaring an API that
only accepts instances of `Sized` and does not need to get the
contents.

> But again, if you are treating these things as containers, then a Sized
> doesn't get you very far, because if you conclude the thing isn't empty,
> you're going to want to get stuff out, and Sized has no methods for
> that.  So presumably there's some companion to Sized for accessing
> elements by index:
>
>  interface HasStuff extends Sized {
>  T get(int index);
>  }

I don't think there has to be. the more useful interface would be this
one, but to date there has been strong resistance in unifying the
Collection and Map interfaces:

 interface Stuff extends Sized {
 int size();
 int isEmpty();
 int isNotEmpty();
Iterator iterator();
 }

Stephen

Re: New Collections interface - Sized

[Stephen Colebourne]

On Fri, 23 Apr 2021 at 17:08, Brian Goetz  wrote:
> This has come up before.  For example, during an early iteration of the 
> Stream design, before parallelism entered the picture.  The first 
> scrawled-on-a-napkin prototype for streams was based on Iterator, and it took 
> about a minute to realize that we could do a much better job if we had a 
> slightly broader interface to work with, essentially Iterator+Sized.
>
> When you pull on this string, you end up with a lot of new interfaces, such 
> as SizedIterator, SizedIterable, etc, in part because ... we have no 
> intersection types.  Having lots of fine-grained interfaces for "has X" and 
> "has Y" is nice from a "bucket of lego bricks" library-design perspective, 
> but when the user goes to express "I need an aggregate that has sizes, 
> iterators, and encounter order", you end up with code like:
>
> > void foo(X x) { ... }
>
> and then you run into the wall of "but I can only use intersection types in 
> these places in the language."  The idiom of having fine-grained mix-in-ish 
> interfaces really wants a language with intersection types.
>
> Additionally, I am having a hard time imagining how Sized would be usable by 
> a client; no method will *take* a Sized (it's just not broad enough), and I 
> can't immediately imagine what would even *return* a Sized.  If the type is 
> not suitable for use by clients, then it serves only to organize the library 
> itself, and that's a weaker motivation.
>
> Is there a compelling example of where this would be used by clients?

Here are some examples:
https://stackoverflow.com/questions/10988634/java-global-isempty-method
https://github.com/OpenGamma/Strata/blob/main/modules/collect/src/main/java/com/opengamma/strata/collect/ArgChecker.java#L404

ie. the ability to spot an "empty" object, or ensure input is
non-empty. Use cases are generally in low-level/framework code like
this, where the actual input data type is not known.

Stephen

New Collections interface - Sized

[Stephen Colebourne]

Hi all,
While a discussion on ReversibleCollection is going on, I'd like to
raise the interface I've always found most missing from the framework
- Sized

 public interface Sized {
   int size();
   default boolean isEmpty() {
 return size() == 0;
   }
   default boolean isNotEmpty() {
 return !isEmpty();
   }
}

This would be applied to Collection and Map, providing a missing
unification. Anything else that has a size could also be retrofitted,
such as String. Ideally arrays too, but that could come later. Note
that Iterable would not implement it.

WDYT?

Stephen

Re: ReversibleCollection proposal

[Stephen Colebourne]

On Fri, 23 Apr 2021 at 07:33, Stuart Marks  wrote:
> On 4/22/21 8:36 AM, Stephen Colebourne wrote:
> Having these methods on Collection could lead to a situation where calling 
> addFirst
> and then getFirst might result in getFirst returning a different element from 
> what
> was passed to addFirst. This doesn't make any sense for methods that have 
> "first" in
> the name.

FWIW, I'm comfortable with that weirdness. The method could be called
addPreferFirst() but that is a bit of a mouthful.

I'd also note that it is much easier for developers of other
Collection implementations to add a method with a matching name than
it is to implement a new interface. Even libraries maintaining
compatibility with Java 5 could do that.

Your proposal has a similar issue BTW - addFirst() on a SortedSet.
Your proposal is to throw UnsupportedOperationException, but I (like
Remi) think that isn't a good answer when
UnsupportedOperationException in collections is usually about
immutability. It is a natural problem of the domain that adding first
when the implementation is sorted is going to be confusing. I am
simply suggesting embracing the weirdness, rather than trying to
exception your way out of it.

Stephen

Re: ReversibleCollection proposal

[Stephen Colebourne]

On Thu, 22 Apr 2021 at 13:58, Remi Forax  wrote:
> I would like to preserve the invariant that, when calling a method on a 
> Collection/iterator, an UnsupportedOperationException only occurs when trying 
> to mutate that collection.
> If we are ok with that, this means that addFirst can not be a default method 
> of Collection.

This implementation meets the invariant:

 public interface Collection  {
   default void addFirst(E e) { add(e); }
   default E getFirst() { return iterator().next(); }
   default E removeFirst() {
 var i = iterator(); i.next();
 i.remove();
   }
 }

This is what I intended anyway, ie that its OK for "first" to work on
an unordered collection, just that addFirst() has very weak semantics
wrt first-ness.

"Ensures that this collection contains the specified element(optional
operation). This has the same basic behaviour as add(E), but
implementations may choose to add the element in first position if
they have some kind of guarantee of ordering. An exception should not
be thrown unless add(E) would also have thrown the exception."

Stephen

Re: ReversibleCollection proposal

[Stephen Colebourne]

On Wed, 21 Apr 2021 at 18:39, Stuart Marks  wrote:
> The value being provided here is that the ReversibleCollection interface 
> provides a
> context within which specs no longer need to hedge about "if the collection 
> has an
> ordering". APIs that produce or consume ReversibleCollection no longer need to
> include this hedge, or have disclaimers about ordering. Potential new
> ReversibleCollection implementations also need not worry about this issue in 
> the
> same way they did when they were forced to implement Collection directly.

Having thought about this for a few days my "interesting thought" is
that adding a `ReversibleCollection` interface and adding additional
methods can be orthogonal choices.

Specifically, I do rather like Peter's suggestion of adding more
methods to Collection. Adding these methods would be pretty useful in
general and give the proposed change much greater reach:

public interface Collection  {
 default void addFirst(E e) { throw new
UnsupportedOperationException(); }
 default E getFirst() { return iterator().next(); }
 default E removeFirst() { var i = iterator(); i.next();
i.remove(); }
}

My view being that every collection has some notion of "first", even
if that notion is "undefined". If that was the only change made, I
think that would be a good move forward.

These would be the various options:

- 7 new methods on ReversibleCollection (Stuart's suggestion)
- 7 new methods on Collection, no ReversibleCollection (Peter's suggestion)
- 3 new methods on Collection, no ReversibleCollection (my suggestion #1)
- 3 new methods on Collection, 4 methods on ReversibleCollection (my
suggestion #2)
- 7 new methods on Collection, 0 methods on ReversibleCollection (my
suggestion #3)

FWIW, I think I prefer my suggestion #2

Stephen

Re: RFR: 8263668: Update java.time to use instanceof pattern variable [v7]

[Stephen Colebourne]

On Wed, 21 Apr 2021 11:06:16 GMT, Patrick Concannon  
wrote:

>> Hi,
>> 
>> Could someone please review my code for updating the code in the `java.time` 
>> package to make use of the `instanceof` pattern variable?
>> 
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request with a new target base due to 
> a merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 10 additional 
> commits since the last revision:
> 
>  - Updated single letter pattern variable name in java/time/Duration
>  - Merge remote-tracking branch 'origin/master' into JDK-8263668
>  - Updated single letter pattern variable names
>  - Merge remote-tracking branch 'origin/master' into JDK-8263668
>  - Merge remote-tracking branch 'origin/master' into JDK-8263668
>  - Merge remote-tracking branch 'origin/master' into JDK-8263668
>  - Merge remote-tracking branch 'origin/master' into JDK-8263668
>  - Merge remote-tracking branch 'origin/master' into JDK-8263668
>  - Merge remote-tracking branch 'origin/master' into JDK-8263668
>  - 8263668: Update java.time to use instanceof pattern variable

Marked as reviewed by scolebourne (Author).

-

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

Re: ReversibleCollection proposal

[Stephen Colebourne]

On Fri, 16 Apr 2021 at 18:41, Stuart Marks  wrote:
> This is a proposal to add a ReversibleCollection interface to the Collections
> Framework. I'm looking for comments on overall design before I work on 
> detailed
> specifications and tests. Please send such comments as replies on this email 
> thread.

I think this could be an interesting addition to the framework.

> # Ordering and Reversibility

Reading this section, it seems like ordering is a more significant
quality than reversibility. Yet the API is named "reversible". That
seems odd, esepcially given the stream characteristic.

> SortedSet::addFirst and addLast throw UnsupportedOperationException. This is 
> because
> SortedSet's ordering is determined by the comparison method and cannot be 
> controlled
> explicitly.

This seems undesirable. Maybe SortedSet should not implement
reversible/ordered? Maybe they should add to the set but validate
whether they would be in first/last position? Simply allowing users to
get a new instance with a different (eg. reversed) comparator would
meet much of the use case.

Also, SortedSet uses first() and last(), yet the proposed interface
uses getFirst() and getLast().

Stephen

Re: Proposal for Decimal64 and Decimal128 value-based classes

[Stephen Colebourne]

On Tue, 30 Mar 2021 at 22:02, Maurizio Cimadamore
 wrote:
> There are also other interesting types to be explored in that domain,
> such as Long128, LongDouble (extended precision float) and HalfFloats.

Perhaps it would be beneficial to have a GitHub repo where designs for
these could be fleshed out. Kind of like a JSR would do. (For example,
Valhalla and OopenJDK will need to agree on naming conventions for the
various methods - for example I prefer plus() but others prefer add().
Stephen

Re: RFR: 8256839: JavaDoc for java.time.Period.negated() method

[Stephen Colebourne]

On Mon, 23 Nov 2020 19:24:51 GMT, Naoto Sato  wrote:

> Hi,
> 
> Please review this simple doc fix. Thanks.

Marked as reviewed by scolebourne (Author).

-

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

Re: RFR: 8247781: Day periods support [v13]

[Stephen Colebourne]

On Thu, 12 Nov 2020 20:03:14 GMT, Naoto Sato  wrote:

>> Hi,
>> 
>> Please review the changes for the subject issue. This is to enhance the 
>> java.time package to support day periods, such as "in the morning", defined 
>> in CLDR. It will add a new pattern character 'B' and its supporting builder 
>> method. The motivation and its spec are in this CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>> 
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed the following comments:
>   - https://github.com/openjdk/jdk/pull/938#discussion_r522185469
>   - https://github.com/openjdk/jdk/pull/938#discussion_r522187931
>   - https://github.com/openjdk/jdk/pull/938#discussion_r522203757
>   - https://github.com/openjdk/jdk/pull/938#discussion_r522211444
>   - https://github.com/openjdk/jdk/pull/938#discussion_r522244221
>   - https://github.com/openjdk/jdk/pull/938#discussion_r522262379
>   - https://github.com/openjdk/jdk/pull/938#discussion_r522266836

Approved with one overflow to fix.

The spec could do with some rewording too. It might be better to explicitly 
mention the "resolving phase" with the three parts:

> The day period is combined with other fields to make a `LocalTime` in the 
> resolving phase. If the `HOUR_OF_AMPM` field is present, it is combined with 
> the day period to make `HOUR_OF_DAY` taking into account any `MINUTE_OF_HOUR` 
> value. If `HOUR_OF_DAY` is present, it is validated against the day period 
> taking into account any `MINUTE_OF_HOUR` value. If a day period is present 
> without `HOUR_OF_DAY`, `MINUTE_OF_HOUR`, `SECOND_OF_MINUTE` and 
> `NANO_OF_SECOND` then the midpoint of the day period is set as the time.

Note that the above is incomplete, and it doesn't describe STRICT/LENIENT, so 
the actual words will be more complex,

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5063:

> 5061: }
> 5062: Long moh = context.getValue(MINUTE_OF_HOUR);
> 5063: long value = (hod * 60 + (moh != null ? moh : 0)) % 1_440;

`long value = Math.floorMod(hod, 24) * 60 + (moh != null ? Math.floorMod(moh, 
60) : 0);`

and remove the next three lines

-

Marked as reviewed by scolebourne (Author).

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

Re: RFR: 8247781: Day periods support [v12]

[Stephen Colebourne]

On Tue, 10 Nov 2020 19:52:14 GMT, Naoto Sato  wrote:

>> Hi,
>> 
>> Please review the changes for the subject issue. This is to enhance the 
>> java.time package to support day periods, such as "in the morning", defined 
>> in CLDR. It will add a new pattern character 'B' and its supporting builder 
>> method. The motivation and its spec are in this CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>> 
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added a test case for user defined temporal field resolution with day 
> period.

src/java.base/share/classes/java/time/format/Parsed.java line 550:

> 548: long midpoint = dayPeriod.mid();
> 549: fieldValues.put(HOUR_OF_DAY, midpoint / 60);
> 550: fieldValues.put(MINUTE_OF_HOUR, midpoint % 60);

Set `dayPeriod = null` here, as it has been successfully used.

src/java.base/share/classes/java/time/format/Parsed.java line 502:

> 500: HOUR_OF_AMPM.checkValidValue(hoap);
> 501: }
> 502: if (dayPeriod.includes((hoap % 24 + 12) * 60)) {

Pretty sure the 24 should be 12.

Also, it should use `Math.floorMod()` to handle `HOUR_OF_AMPM = -1` (which is 
11 o'clock).

Also, this doesn't take into account minutes. So if the day period rule is 
afternoon1 to 18:30 and evening1 after 18:30, and the input is `HOUR_OF_AMPM = 
6, MINUTE_OF_HOUR = 45`, this will fail to resolve,

Something like this should work (no need for `updateCheckDayPeriodConflict`):

long hoap = fieldValues.remove(HOUR_OF_AMPM);
if (resolverStyle == ResolverStyle.LENIENT) {
  HOUR_OF_AMPM.checkValidValue(hoap);
}
Long mohObj = fieldValues.get(MINUTE_OF_HOUR);
long moh = mohObj != null ? Math.floorMod(mohObj, 60) : 0;
long excessHours = dayPeriod.includes(Math.floorMod(hoap, 12) + 12 * 60 + 
moh ? 12 : 0;
long hod = Math.addExact(hoap, excessHours);
updateCheckConflict(HOUR_OF_AMPM, HOUR_OF_DAY, hod);
dayPeriod = null;

src/java.base/share/classes/java/time/format/Parsed.java line 546:

> 544: 
> 545: // Set the hour-of-day, if not exist and not in STRICT, to 
> the mid point of the day period or am/pm.
> 546: if (!fieldValues.containsKey(HOUR_OF_DAY) && resolverStyle 
> != ResolverStyle.STRICT) {

Since this logic replaces both `HOUR_OF_DAY` and `MINUTE_OF_HOUR` I think it 
should only be invoked if both do not exist. Beyond that, it doesn't really 
make sense to do this logic if second/sub-second are present. Thus, it needs to 
check all four fields and can then directly set the time.

if (!fieldValues.containsKey(HOUR_OF_DAY) && 
!fieldValues.containsKey(MINUTE_OF_HOUR) && 
!fieldValues.containsKey(SECOND_OF_MINUTE) && 
!fieldValues.containsKey(NANO_OF_SECOND) && 
resolverStyle != ResolverStyle.STRICT) {

  if (dayPeriod != null) {
long midpoint = dayPeriod.mid();
resolveTime(midpoint / 60, midpoint % 60, 0, 0);
dayPeriod = null;
  } else if (fieldValues.containsKey(AMPM_OF_DAY)) {
long ap = fieldValues.remove(AMPM_OF_DAY);
if (resolverStyle == ResolverStyle.LENIENT) {
  resolveTime(Math.addExact(Math.multiplyExact(ap, 12), 6), 0, 0, 0);
} else {  // SMART
  AMPM_OF_DAY.checkValidValue(ap);
  resolveTime(ap * 12 + 6, 0, 0, 0);
}

src/java.base/share/classes/java/time/format/Parsed.java line 568:

> 566: if (dayPeriod != null) {
> 567: // Check whether the hod is within the day period
> 568: updateCheckDayPeriodConflict(HOUR_OF_DAY, hod);

With the other changes, this is the only use of this method and it can 
therefore be simplified (no need to check for conflicts, just whether it 
matches the day period).

test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line 
778:

> 776: {"h B", ResolverStyle.SMART, "59 in the morning", 11},
> 777: 
> 778: {"H B", ResolverStyle.LENIENT, "47 at night", 23},

This test should be split in two - smart (fails) and lenient (succeeds). The 
lenient tests should ensure that 
`p.query(DateTimeFormatter.parsedExcessDays())` returns the expected number of 
excess days. 

I'd also add a test for a negative value such as "-2 at night"

-

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

Re: RFR: 8247781: Day periods support [v9]

[Stephen Colebourne]

On Thu, 5 Nov 2020 23:24:19 GMT, Stephen Colebourne  
wrote:

>> I implemented what you suggested here in the latest PR, but that would be a 
>> behavioral change which requires a CSR, as "AM" would be resolved to 06:00 
>> which was not before. Do you think it would be acceptable? If so, I will 
>> reopen the CSR and describe the change. (In fact some TCK failed with this 
>> impl)
>
> I find the whole "half way between the start and end" behaviour of day 
> periods odd anyway, but if it is to be supported then it should be consistent 
> as you've implemented. 
> 
> Another option I should have thought of earlier would be to simply not 
> support the "half way between the start and end" behaviour of LDML in either 
> dayPeriod or AM/PM. But since LDML defines it, you've implemented it, and it 
> isn't overly harmful I think its OK to leave it in.
> 
> Would it be possible for STRICT mode to not have the "half way between the 
> start and end" behaviour?

I've had a look tonight, but found two more problems!

The comments at the start of `resolveTimeLenient()` indicate that setting the 
midpoint in `resolveTimeFields()` is a bad idea - it needs to be done inside 
`resolveTimeLenient()`. (This ensures user-defined fields can resolve to 
ChronoFields IIRC). Thus, the day period resolving would have to be split 
between the two methods. How important is the midpoint logic? Could it just be 
dropped?

Secondly, we need to ensure that "24:00 midnight" (using HOUR_OF_DAY only) 
correctly parses to the end of day midnight, not the start of day or an 
exception. This is related to the problem above. 

I _think_ (but haven't confirmed everything yet) that the only thing that 
should be in `resolveTimeFields()` is the resolution of HOUR_OF_AMPM + 
dayPeriod to HOUR_OF_DAY (with `dayPeriod` then being set to `null`). 
Everything else should go in `resolveTimeLenient()` - the midpoint logic in the 
first if block (where time fields are defaulted), and the validation logic at 
the end of the method.

-

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

Re: RFR: 8247781: Day periods support [v7]

[Stephen Colebourne]

On Fri, 6 Nov 2020 03:00:52 GMT, Naoto Sato  wrote:

>> test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line 
>> 858:
>> 
>>> 856: return new Object[][]{
>>> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
>>> 858: {STRICT, 1, LocalTime.of(18, 0), 1},
>> 
>> As mentioned in my other comment, this seems odd in STRICT mode.
>
> Did you mean in STRICT mode, HOUR_OF_AMPM should default to 0, and to 6 in 
> SMART/LENIENT modes?

No. I mean that when resolving AMPM/dayPeriod in strict mode, and there is no 
HOUR_OF_DAY or HOUR_OF_AMPM, then do not resolve using "half way between"(ie. 
fail). This will produce a result where `LocalTime` cannot be obtained.

var f = 
DateTimeFormatter.ofPattern("B").withResolverStyle(ResolverStyle.STRICT);
var t = LocalTime.from("at night", f);
would throw an exception in STRICT mode (whereas SMART or LENIENT would return 
a `LocalTime`). Same with pattern "a".

-

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

Re: RFR: 8247781: Day periods support [v7]

[Stephen Colebourne]

On Thu, 5 Nov 2020 17:12:11 GMT, Naoto Sato  wrote:

>> Hi,
>> 
>> Please review the changes for the subject issue. This is to enhance the 
>> java.time package to support day periods, such as "in the morning", defined 
>> in CLDR. It will add a new pattern character 'B' and its supporting builder 
>> method. The motivation and its spec are in this CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>> 
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed typo/grammatical error.

test/jdk/java/time/tck/java/time/format/TCKDateTimeParseResolver.java line 858:

> 856: return new Object[][]{
> 857: {STRICT, 0, LocalTime.of(6, 0), 0},
> 858: {STRICT, 1, LocalTime.of(18, 0), 1},

As mentioned in my other comment, this seems odd in STRICT mode.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5055:

> 5053: @Override
> 5054: public boolean format(DateTimePrintContext context, 
> StringBuilder buf) {
> 5055: Long value = context.getValue(MINUTE_OF_DAY);

This does not match the spec: " During formatting, the day period is obtained 
from {@code HOUR_OF_DAY}, and optionally {@code MINUTE_OF_HOUR} if exist"

It is possible and legal to create a Temporal that returns `HOUR_OF_DAY` and 
`MINUTE_OF_HOUR` but not `MINUTE_OF_DAY`. As such, this method must be changed 
to follow the spec.

-

In addition, it is possible for `HOUR_OF_DAY` and `MINUTE_OF_HOUR` to be 
outside their normal bounds. The right behaviour would be to combine the two 
fields within this method, and then use mod to get the value into the range 0 
to 1440 before calling `dayPeriod.include`. (While the fall back behaviour 
below does cover this, it would be better to do what I propose here.)

An example of this is a `TransportTime` class where the day runs from 03:00 to 
27:00 each day (because trains run after midnight for no extra cost to the 
passenger, and it is more convenient for the operator to treat the date that 
way). A `TransportTime` of 26:30 should still resolve to "night1" rather than 
fall back to "am".

-

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

Re: RFR: 8247781: Day periods support [v7]

[Stephen Colebourne]

On Mon, 2 Nov 2020 23:21:22 GMT, Naoto Sato  wrote:

>> Pulling on this a little more.
>> 
>> As the PR stands, it seems that if the user passes in text with just a 
>> day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in 
>> `AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? 
>> If so, I don't think this is sustainable.
>> 
>> Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of 
>> "am" or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then 
>> dayPeriod can silently take precedence
>
> I implemented what you suggested here in the latest PR, but that would be a 
> behavioral change which requires a CSR, as "AM" would be resolved to 06:00 
> which was not before. Do you think it would be acceptable? If so, I will 
> reopen the CSR and describe the change. (In fact some TCK failed with this 
> impl)

I find the whole "half way between the start and end" behaviour of day periods 
odd anyway, but if it is to be supported then it should be consistent as you've 
implemented. 

Another option I should have thought of earlier would be to simply not support 
the "half way between the start and end" behaviour of LDML in either dayPeriod 
or AM/PM. But since LDML defines it, you've implemented it, and it isn't overly 
harmful I think its OK to leave it in.

Would it be possible for STRICT mode to not have the "half way between the 
start and end" behaviour?

-

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

Re: RFR: 8180352: Add Stream.toList() method

[Stephen Colebourne]

On Tue, 3 Nov 2020 01:33:32 GMT, Stuart Marks  wrote:

> This change introduces a new terminal operation on Stream. This looks like a 
> convenience method for Stream.collect(Collectors.toList()) or 
> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
> method directly on Stream enables it to do what can't easily by done by a 
> Collector. In particular, it allows the stream to deposit results directly 
> into a destination array (even in parallel) and have this array be wrapped in 
> an unmodifiable List without copying.
> 
> In the past we've kept most things from the Collections Framework as 
> implementations of Collector, not directly on Stream, whereas only 
> fundamental things (like toArray) appear directly on Stream. This is true of 
> most Collections, but it does seem that List is special. It can be a thin 
> wrapper around an array; it can handle generics better than arrays; and 
> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
> can be value-based. See John Rose's comments in the bug report:
> 
> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
> 
> This operation is null-tolerant, which matches the rest of Streams. This 
> isn't specified, though; a general statement about null handling in Streams 
> is probably warranted at some point.
> 
> Finally, this method is indeed quite convenient (if the caller can deal with 
> what this operation returns), as collecting into a List is the most common 
> stream terminal operation.

src/java.base/share/classes/java/util/ImmutableCollections.java line 211:

> 209: }
> 210: 
> 211: switch (input.length) { // implicit null check of elements

Was a variable renamed? Should "elements" be "input"?

src/java.base/share/classes/java/util/stream/Stream.java line 1168:

> 1166:  * Accumulates the elements of this stream into a {@code List}. The 
> elements in
> 1167:  * the list will be in this stream's encounter order, if one 
> exists. There are no
> 1168:  * guarantees on the implementation type, mutability, 
> serializability, or

It would be useful for callers to feel more confident that they will get an 
immutable instance. In java.time.* we have wording like "This interface places 
no restrictions on the mutability of implementations, however immutability is 
strongly recommended." Could something like that work here, emphasising that 
everyone implementing this method should seek to return an immutable list?

-

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

Re: RFR: 8247781: Day periods support [v3]

[Stephen Colebourne]

On Fri, 30 Oct 2020 22:03:08 GMT, Naoto Sato  wrote:

>> Hi,
>> 
>> Please review the changes for the subject issue. This is to enhance the 
>> java.time package to support day periods, such as "in the morning", defined 
>> in CLDR. It will add a new pattern character 'B' and its supporting builder 
>> method. The motivation and its spec are in this CSR:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8254629
>> 
>> Naoto
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fixed exception messages.

test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line 
656:

> 654: {"h B", "11 at night", 23},
> 655: {"h B", "3 at night", 3},
> 656: {"h B", "11 in the morning", 11},

Need tests for "51 in the morning" (which should parse in LENIENT as "3 in the 
morning" plus 2 days, see how HOUR_OF_DAY=51 works in general.

Similar issue with HOUR_OF_AMPM=3 and AMPM_OF_DAY=4.

-

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

Re: RFR: 8247781: Day periods support [v3]

[Stephen Colebourne]

On Fri, 30 Oct 2020 11:06:59 GMT, Stephen Colebourne  
wrote:

>> Naoto Sato has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Fixed exception messages.
>
> src/java.base/share/classes/java/time/format/Parsed.java line 497:
> 
>> 495: AMPM_OF_DAY.checkValidValue(ap);
>> 496: }
>> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint 
>> / 720);
> 
> No need to put `AMPM_OF_DAY` back in here because you've already resolved it 
> to `HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be 
> validation to check that the day period agrees with the AM/PM value.

Line can still be removed AFAICT

-

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

Re: RFR: 8247781: Day periods support [v3]

[Stephen Colebourne]

On Fri, 30 Oct 2020 21:30:50 GMT, Naoto Sato  wrote:

>> src/java.base/share/classes/java/time/format/Parsed.java line 472:
>> 
>>> 470: }
>>> 471: if (dayPeriod != null) {
>>> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {
>> 
>> Are we certain that the CLDR data does not contain day periods based on 
>> minutes as well as hours? This logic does not check against MINUTE_OF_HOUR 
>> for example. The logic also conflicts with the spec Javadoc that says 
>> MINUTE_OF_HOUR is validated.
>
> MINUTE_OF_HOUR without HOUR_OF_DAY/HOUR_OF_AMPM may not make any sense, so it 
> is only validated in updateCheckDayPeriodConflict.

But can you get a CLDR rule that says "at night" before 05:30 and "In the 
morning" from 05:30 onwards? If you can then I don't think it is handled, 
because HOUR_OF_DAY and MINUTE_OF_HOUR are not used together when checking 
against DayPeriod.

>> src/java.base/share/classes/java/time/format/Parsed.java line 500:
>> 
>>> 498: }
>>> 499: }
>>> 500: }
>> 
>> Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored 
>> if there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check 
>> `AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and 
>> HOUR_OF_DAY=18 does not look like it throws an exception, when it probably 
>> should).
>> 
>> On solution would be for `AMPM_OF_DAY` to be resolved to a day period at 
>> line 427, checking for conflicts with any parsed day period. (a small bug 
>> fix behavioural change)
>
> There are cases where a period crosses midnight, e.g., 23:00-04:00 so it 
> cannot validate am/pm, so I decided to just override ampm with dayperiod 
> without validating.

Pulling on this a little more.

As the PR stands, it seems that if the user passes in text with just a 
day-period of "AM" they get a `LocalTime` of 06:00 but if they pass in 
`AMPM_OF_DAY` of "AM" the get no `LocalTime` in the result. Is that right? If 
so, I don't think this is sustainable.

Thats why I think `AMPM_OF_DAY` will have to be resolved to a dayPeriod of "am" 
or "pm". If dayPeriod is more precise than `AMPM_OF_DAY`, then dayPeriod can 
silently take precedence

-

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

Re: RFR: 8247781: Day periods support

[Stephen Colebourne]

On Thu, 29 Oct 2020 15:59:51 GMT, Naoto Sato  wrote:

> Hi,
> 
> Please review the changes for the subject issue. This is to enhance the 
> java.time package to support day periods, such as "in the morning", defined 
> in CLDR. It will add a new pattern character 'B' and its supporting builder 
> method. The motivation and its spec are in this CSR:
> 
> https://bugs.openjdk.java.net/browse/JDK-8254629
> 
> Naoto

Changes requested by scolebourne (Author).

test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line 
981:

> 979: 
> 980: {"B", "Text(DayPeriod,SHORT)"},
> 981: {"BB", "Text(DayPeriod,SHORT)"},

"BB" and "BBB" are not defined to do anything in the CSR. Java should match 
CLDR/LDML rules here.

test/jdk/java/time/test/java/time/format/TestDateTimeFormatterBuilder.java line 
540:

> 538: builder.appendDayPeriodText(TextStyle.FULL);
> 539: DateTimeFormatter f = builder.toFormatter();
> 540: assertEquals(f.toString(), "Text(DayPeriod,FULL)");

This should be "DayPeriod(FULL)", because an end user might create a 
`TemporalField` with the name "DayPeriod" and the toString should be unique.

src/java.base/share/classes/java/time/format/Parsed.java line 352:

> 350: (fieldValues.containsKey(MINUTE_OF_HOUR) ? 
> fieldValues.get(MINUTE_OF_HOUR) : 0);
> 351: if (!dayPeriod.includes(mod)) {
> 352: throw new DateTimeException("Conflict found: " + 
> changeField + " conflict with day period");

"conflict with day period" -> "conflicts with day period"

Should also include `changeValue` and ideally the valid range

src/java.base/share/classes/java/time/format/Parsed.java line 472:

> 470: }
> 471: if (dayPeriod != null) {
> 472: if (fieldValues.containsKey(HOUR_OF_DAY)) {

Are we certain that the CLDR data does not contain day periods based on minutes 
as well as hours? This logic does not check against MINUTE_OF_HOUR for example. 
The logic also conflicts with the spec Javadoc that says MINUTE_OF_HOUR is 
validated.

src/java.base/share/classes/java/time/format/Parsed.java line 497:

> 495: AMPM_OF_DAY.checkValidValue(ap);
> 496: }
> 497: updateCheckDayPeriodConflict(AMPM_OF_DAY, midpoint / 
> 720);

No need to put `AMPM_OF_DAY` back in here because you've already resolved it to 
`HOUR_OF_DAY` and `MINUTE_OF_HOUR`. There probably does need to be validation 
to check that the day period agrees with the AM/PM value.

src/java.base/share/classes/java/time/format/Parsed.java line 500:

> 498: }
> 499: }
> 500: }

Looking at the existing logic, the `AMPM_OF_DAY` field is completely ignored if 
there is no `HOUR_OF_AMPM` field. Thus, there is no validation to check 
`AMPM_OF_DAY` against `HOUR_OF_DAY`. This seems odd. (AMPM_OF_DAY = 0 and 
HOUR_OF_DAY=18 does not look like it throws an exception, when it probably 
should).

On solution would be for `AMPM_OF_DAY` to be resolved to a day period at line 
427, checking for conflicts with any parsed day period. (a small bug fix 
behavioural change)

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
1489:

> 1487: Objects.requireNonNull(style, "style");
> 1488: if (style != TextStyle.FULL && style != TextStyle.SHORT && 
> style != TextStyle.NARROW) {
> 1489: throw new IllegalArgumentException("Style must be either 
> full, short, or narrow");

Nothing in the Javadoc describes this behaviour. It would make more sense to 
map FULL_STANDALONE to FULL and so on and not throw an exception.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
1869:

> 1867: } else if (cur == 'B') {
> 1868: switch (count) {
> 1869: case 1, 2, 3 -> 
> appendDayPeriodText(TextStyle.SHORT);

I think this should be `case 1`. The 2 and 3 are not in the Javadoc, and I 
don't think they are in LDML. I note that patterns G and E do this though, so 
there is precedent.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5094:

> 5092: @Override
> 5093: public String toString() {
> 5094: return "Text(DayPeriod," + textStyle + ")";

Should be "DayPeriod(FULL)" to avoid possible `toString` clashes with the text 
printer/parser

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5153:

> 5151:  * minute-of-day of "at" or "from" attribute
> 5152:  */
> 5153: private final long from;

Could these three instance variables be `int` ?

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 
5252:

> 5250: 
> .getLocaleResources(CalendarDataUtility.findRegionOverride(l));
> 5251: String dayPeriodRules = lr.getRules()[1];
> 5252: final 

Re: RFR 8233048: WeekFields.ISO is not a singleton

[Stephen Colebourne]

 +1
Stephen

On Thu, 30 Jul 2020 at 22:54,  wrote:
>
> Hi,
>
> Please review the fix to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8233048
>
> The proposed chageset is located at:
>
> https://cr.openjdk.java.net/~naoto/8233048/webrev.00/
>
> java.time.temporal.WeekFields.ISO is instantiated using the constructor,
> which is a different object returned from WeekFields.of(MONDAY, 4). Even
> though this itself does not cause any issues, since objects are "equal",
> but the fix will provide the identity, as WeekFields objects are
> supposed to be singletons.
>
> Naoto
>

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[Stephen Colebourne]

I'm not an official OpenJDK reviewer, nor would I be confident
reviewing the native code here.
Stephen

On Tue, 26 May 2020 at 14:28, David Holmes  wrote:
>
> On 26/05/2020 6:14 pm, Stephen Colebourne wrote:
> > AFAICT a nanosecond clock is fine from a java.time.* API perspective.
>
> Thanks Stephen. Is this a review or just a nod of approval? :)
>
> Cheers,
> David
> -
>
> > Stephen
> >
> > On Tue, 26 May 2020 at 06:01, David Holmes  wrote:
> >>
> >> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> >> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
> >>
> >> This work was contributed by Mark Kralj-Taylor:
> >>
> >> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html
> >>
> >> On the hotspot side we change the Linux implementations of
> >> javaTimeMillis() and javaTimeSystemUTC() so that they use
> >> clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with
> >> our conditional use of this API I added a new guard
> >>
> >> os::Posix::supports_clock_gettime()
> >>
> >> and refined the existing supports_monotonic_clock() so that we can still
> >> use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future
> >> (hopefully very near future) we will simply assume these APIs always exist.
> >>
> >> On the core-libs side the existing test:
> >>
> >> test/jdk/java/time/test/java/time/TestClock_System.java
> >>
> >> is adjusted to track the precision in more detail.
> >>
> >> Finally Mark has added instantNowAsEpochNanos() to the benchmark
> >> previously known as
> >>
> >> test/micro/org/openjdk/bench/java/lang/Systems.java
> >>
> >> which I've renamed to SystemTime.java as Mark suggested. I agree having
> >> these three functions measured together makes sense.
> >>
> >> Testing:
> >> - test/jdk/java/time/test/java/time/TestClock_System.java
> >> - test/micro/org/openjdk/bench/java/lang/SystemTime.java
> >> - Tiers 1-3
> >>
> >> Thanks,
> >> David

Re: RFR(S) 8242504: Enhance the system clock to nanosecond precision

[Stephen Colebourne]

AFAICT a nanosecond clock is fine from a java.time.* API perspective.
Stephen

On Tue, 26 May 2020 at 06:01, David Holmes  wrote:
>
> bug: https://bugs.openjdk.java.net/browse/JDK-8242504
> webrev: http://cr.openjdk.java.net/~dholmes/8242504/webrev/
>
> This work was contributed by Mark Kralj-Taylor:
>
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-April/038975.html
>
> On the hotspot side we change the Linux implementations of
> javaTimeMillis() and javaTimeSystemUTC() so that they use
> clock_gettime(CLOCK_REALTIME) instead of gettimeofday(). In keeping with
> our conditional use of this API I added a new guard
>
> os::Posix::supports_clock_gettime()
>
> and refined the existing supports_monotonic_clock() so that we can still
> use CLOCK_REALTIME if CLOCK_MONOTONIC does not exist. In the future
> (hopefully very near future) we will simply assume these APIs always exist.
>
> On the core-libs side the existing test:
>
> test/jdk/java/time/test/java/time/TestClock_System.java
>
> is adjusted to track the precision in more detail.
>
> Finally Mark has added instantNowAsEpochNanos() to the benchmark
> previously known as
>
> test/micro/org/openjdk/bench/java/lang/Systems.java
>
> which I've renamed to SystemTime.java as Mark suggested. I agree having
> these three functions measured together makes sense.
>
> Testing:
>- test/jdk/java/time/test/java/time/TestClock_System.java
>- test/micro/org/openjdk/bench/java/lang/SystemTime.java
>- Tiers 1-3
>
> Thanks,
> David

Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values

[Stephen Colebourne]

On Thu, 7 May 2020 at 07:38, Joe Wang  wrote:
> The Javadoc states:
>  If the locale contains the "ca" (calendar), "nu" (numbering
> system), "rg" (region override), and/or "tz" (timezone) Unicode
> extensions, the chronology, numbering system and/or the zone are overridden.
>
> If you remove the two statements that check whether the specified locale
> contains "ca" or "nu", would you need to update the Javadoc as well?

Those things are checked by the methods Chronology.of(locale) and
DecimalStyle.of(locale), so although indirect, I think the Javadoc is
fine.
Stephen

Re: [15]: RFR: 8244245: localizedBy() should override localized values with default values

[Stephen Colebourne]

+1
Stephen

On Wed, 6 May 2020 at 21:50,  wrote:
>
> Hello,
>
> Please review the fix to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8244245
>
> The CSR and proposed changeset are located at:
>
> https://bugs.openjdk.java.net/browse/JDK-8244246
> https://cr.openjdk.java.net/~naoto/8244245/webrev.00/
>
> This stems from the closed issue
> (https://bugs.openjdk.java.net/browse/JDK-8243162), and the rationale
> for this fix is discussed there.
>
> Naoto
>

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

[Stephen Colebourne]

Fine by me, but I'm not an OpenJDK Reviewer
Stephen

On Mon, 9 Mar 2020 at 03:05,  wrote:
>
> Thanks, Stephen.
>
> Updated the webrev for those two suggestions:
>
> http://cr.openjdk.java.net/~naoto/8239836/webrev.04/
>
> Naoto
>
> On 3/8/20 4:22 PM, Stephen Colebourne wrote:
> >   standardOffsets[0] == wallOffsets[0] needs to use .equals()
> >
> > Unrelated, there is a Javadoc/spec error at line 578 of the "new"
> > file. It should be getValidOffsets, not getOffset.
> >
> > Apart from that, it looks good.
> >
> > thanks
> > Stephen
> >
> >
> > On Wed, 4 Mar 2020 at 19:06,  wrote:
> >>
> >> Hi Stephen,
> >>
> >> Thank you for the detailed explanation and correcting the
> >> implementation. I incorporated all the suggestions below, as well as
> >> adding a new test for isFixedOffset() implementation (with some clean-up):
> >>
> >> https://cr.openjdk.java.net/~naoto/8239836/webrev.03/
> >>
> >> Please take a look at it.
> >>
> >> Naoto
> >>
> >> On 3/3/20 3:30 PM, Stephen Colebourne wrote:
> >>> I fear more changes are needed here.
> >>>
> >>> 1) The code is isFixedOffset() is indeed wrong, but the fix is
> >>> insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
> >>> and all three other lists are empty. A fixed offset rule is the
> >>> equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
> >>> assumes that.
> >>>
> >>> 2) The code in getOffset(Instant) is wrong, but so is the proposed
> >>> fix. The purpose of the if statement is to optimise the following few
> >>> lines which refer to savingsInstantTransitions and wallOffsets. The
> >>> code does have a bug as it should return the first wall offset.
> >>> Corrected code:
> >>> public ZoneOffset getOffset(Instant instant) {
> >>>   if (savingsInstantTransitions.length == 0) {
> >>> return wallOffsets[0];
> >>>   }
> >>> With the correct definition of isFixedOffset() it becomes apparent
> >>> that this if statement is in fact not related to the fixed offset.
> >>>
> >>> Currently this test case fails (a zone in DST for all eternity):
> >>>   ZoneRules rules = ZoneRules.of(
> >>>   ZoneOffset.ofHours(1),
> >>>   ZoneOffset.ofHours(2),
> >>>   Collections.emptyList(),
> >>>   Collections.emptyList(),
> >>>   Collections.emptyList());
> >>>   assertEquals(rules.getStandardOffset(Instant.EPOCH),
> >>> ZoneOffset.ofHours(1));
> >>>   assertEquals(rules.getOffset(Instant.EPOCH),  
> >>> ZoneOffset.ofHours(2));
> >>>
> >>> 3) The code in getStandardOffset(Instant) also has an error as it
> >>> checks the wrong array. It should check standardTransitions as that is
> >>> what is used in the following few lines. Corrected code:
> >>> public ZoneOffset getStandardOffset(Instant instant) {
> >>>   if (standardTransitions.length == 0) {
> >>> return standardOffsets[0];
> >>>   }
> >>>
> >>> 4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
> >>> Since it protects savingsLocalTransitions, it should return
> >>> wallOffsets[0].
> >>>
> >>> 5) The code in getDaylightSavings(Instant instant) has an optimising
> >>> if statement that should refer to isFixedOffset().
> >>>
> >>> 6) Also, in the test.
> >>>assertTrue(!rules.isFixedOffset());
> >>> should be
> >>>assertFalse(rules.isFixedOffset());
> >>>
> >>> The class has worked so far as every normal case has
> >>> baseStandardOffset = baseWallOffset, even though it is conceptually
> >>> valid for this not to be true.
> >>>
> >>> Stephen
> >>>
> >>>
> >>>
> >>>
> >>> On Thu, 27 Feb 2020 at 20:42,  wrote:
> >>>>
> >>>> Hello,
> >>>>
> >>>> Please review the fix to the following issue:
> >>>>
> >>>> https://bugs.openjdk.java.net/browse/JDK-8239836
> >>>>
> >>>> The proposed changeset is located at:
> >>>>
> >>>> https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
> >>>>
> >>>> It turned out that 'transitionList' is not necessarily a superset of
> >>>> 'standardOffsetTransitionList', as in some occasions where a standard
> >>>> offset change and a savings change happen at the same time (canceling
> >>>> each other), resulting in no wall time transition. This means that the
> >>>> "rules" in the sample code is a valid ZoneRules instance.
> >>>>
> >>>> However, there is an assumption in ZoneRules where no (wall time)
> >>>> transition means the fixed offset zone. With that assumption, the
> >>>> example rule is considered a fixed offset zone which is not correct. So
> >>>> the fix proposed here is not to throw an IAE but to recognize the rule
> >>>> as a valid, non-fixed offset ZoneRules instance.
> >>>>
> >>>> Naoto
> >>>>
> >>>>

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

[Stephen Colebourne]

 standardOffsets[0] == wallOffsets[0] needs to use .equals()

Unrelated, there is a Javadoc/spec error at line 578 of the "new"
file. It should be getValidOffsets, not getOffset.

Apart from that, it looks good.

thanks
Stephen


On Wed, 4 Mar 2020 at 19:06,  wrote:
>
> Hi Stephen,
>
> Thank you for the detailed explanation and correcting the
> implementation. I incorporated all the suggestions below, as well as
> adding a new test for isFixedOffset() implementation (with some clean-up):
>
> https://cr.openjdk.java.net/~naoto/8239836/webrev.03/
>
> Please take a look at it.
>
> Naoto
>
> On 3/3/20 3:30 PM, Stephen Colebourne wrote:
> > I fear more changes are needed here.
> >
> > 1) The code is isFixedOffset() is indeed wrong, but the fix is
> > insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
> > and all three other lists are empty. A fixed offset rule is the
> > equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
> > assumes that.
> >
> > 2) The code in getOffset(Instant) is wrong, but so is the proposed
> > fix. The purpose of the if statement is to optimise the following few
> > lines which refer to savingsInstantTransitions and wallOffsets. The
> > code does have a bug as it should return the first wall offset.
> > Corrected code:
> >public ZoneOffset getOffset(Instant instant) {
> >  if (savingsInstantTransitions.length == 0) {
> >return wallOffsets[0];
> >  }
> > With the correct definition of isFixedOffset() it becomes apparent
> > that this if statement is in fact not related to the fixed offset.
> >
> > Currently this test case fails (a zone in DST for all eternity):
> >  ZoneRules rules = ZoneRules.of(
> >  ZoneOffset.ofHours(1),
> >  ZoneOffset.ofHours(2),
> >  Collections.emptyList(),
> >  Collections.emptyList(),
> >  Collections.emptyList());
> >  assertEquals(rules.getStandardOffset(Instant.EPOCH),
> > ZoneOffset.ofHours(1));
> >  assertEquals(rules.getOffset(Instant.EPOCH),  
> > ZoneOffset.ofHours(2));
> >
> > 3) The code in getStandardOffset(Instant) also has an error as it
> > checks the wrong array. It should check standardTransitions as that is
> > what is used in the following few lines. Corrected code:
> >public ZoneOffset getStandardOffset(Instant instant) {
> >  if (standardTransitions.length == 0) {
> >return standardOffsets[0];
> >  }
> >
> > 4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
> > Since it protects savingsLocalTransitions, it should return
> > wallOffsets[0].
> >
> > 5) The code in getDaylightSavings(Instant instant) has an optimising
> > if statement that should refer to isFixedOffset().
> >
> > 6) Also, in the test.
> >   assertTrue(!rules.isFixedOffset());
> > should be
> >   assertFalse(rules.isFixedOffset());
> >
> > The class has worked so far as every normal case has
> > baseStandardOffset = baseWallOffset, even though it is conceptually
> > valid for this not to be true.
> >
> > Stephen
> >
> >
> >
> >
> > On Thu, 27 Feb 2020 at 20:42,  wrote:
> >>
> >> Hello,
> >>
> >> Please review the fix to the following issue:
> >>
> >> https://bugs.openjdk.java.net/browse/JDK-8239836
> >>
> >> The proposed changeset is located at:
> >>
> >> https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
> >>
> >> It turned out that 'transitionList' is not necessarily a superset of
> >> 'standardOffsetTransitionList', as in some occasions where a standard
> >> offset change and a savings change happen at the same time (canceling
> >> each other), resulting in no wall time transition. This means that the
> >> "rules" in the sample code is a valid ZoneRules instance.
> >>
> >> However, there is an assumption in ZoneRules where no (wall time)
> >> transition means the fixed offset zone. With that assumption, the
> >> example rule is considered a fixed offset zone which is not correct. So
> >> the fix proposed here is not to throw an IAE but to recognize the rule
> >> as a valid, non-fixed offset ZoneRules instance.
> >>
> >> Naoto
> >>
> >>

Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

[Stephen Colebourne]

I fear more changes are needed here.

1) The code is isFixedOffset() is indeed wrong, but the fix is
insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
and all three other lists are empty. A fixed offset rule is the
equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
assumes that.

2) The code in getOffset(Instant) is wrong, but so is the proposed
fix. The purpose of the if statement is to optimise the following few
lines which refer to savingsInstantTransitions and wallOffsets. The
code does have a bug as it should return the first wall offset.
Corrected code:
  public ZoneOffset getOffset(Instant instant) {
if (savingsInstantTransitions.length == 0) {
  return wallOffsets[0];
}
With the correct definition of isFixedOffset() it becomes apparent
that this if statement is in fact not related to the fixed offset.

Currently this test case fails (a zone in DST for all eternity):
ZoneRules rules = ZoneRules.of(
ZoneOffset.ofHours(1),
ZoneOffset.ofHours(2),
Collections.emptyList(),
Collections.emptyList(),
Collections.emptyList());
assertEquals(rules.getStandardOffset(Instant.EPOCH),
ZoneOffset.ofHours(1));
assertEquals(rules.getOffset(Instant.EPOCH),  ZoneOffset.ofHours(2));

3) The code in getStandardOffset(Instant) also has an error as it
checks the wrong array. It should check standardTransitions as that is
what is used in the following few lines. Corrected code:
  public ZoneOffset getStandardOffset(Instant instant) {
if (standardTransitions.length == 0) {
  return standardOffsets[0];
}

4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
Since it protects savingsLocalTransitions, it should return
wallOffsets[0].

5) The code in getDaylightSavings(Instant instant) has an optimising
if statement that should refer to isFixedOffset().

6) Also, in the test.
 assertTrue(!rules.isFixedOffset());
should be
 assertFalse(rules.isFixedOffset());

The class has worked so far as every normal case has
baseStandardOffset = baseWallOffset, even though it is conceptually
valid for this not to be true.

Stephen




On Thu, 27 Feb 2020 at 20:42,  wrote:
>
> Hello,
>
> Please review the fix to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8239836
>
> The proposed changeset is located at:
>
> https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
>
> It turned out that 'transitionList' is not necessarily a superset of
> 'standardOffsetTransitionList', as in some occasions where a standard
> offset change and a savings change happen at the same time (canceling
> each other), resulting in no wall time transition. This means that the
> "rules" in the sample code is a valid ZoneRules instance.
>
> However, there is an assumption in ZoneRules where no (wall time)
> transition means the fixed offset zone. With that assumption, the
> example rule is considered a fixed offset zone which is not correct. So
> the fix proposed here is not to throw an IAE but to recognize the rule
> as a valid, non-fixed offset ZoneRules instance.
>
> Naoto
>
>

Re: [15] RFR: 8236903: ZoneRules#getOffset throws DateTimeException for rules with last rules

[Stephen Colebourne]

Looks good
Stephen

On Tue, 21 Jan 2020 at 21:53,  wrote:
> Please review the fix to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8236903
>
> The proposed changeset is located at:
>
> http://cr.openjdk.java.net/~naoto/8236903/webrev.00/
>
> This edge case throws an out-of-bounds DateTimeException in
> ZoneRules.findYear() on calculating the year. Instead of relying on
> LocalDate.ofEpochDay(), inlining the year caluculation from that method,
> and cap to the MAX value if needed, will solve the issue.
>
> Naoto

Re: [14] RFR: 8230136: DateTimeFormatterBuilder.FractionPrinterParser#parse fails to verify minWidth

[Stephen Colebourne]

+1

On Wed, 11 Sep 2019 at 13:38,  wrote:
>
> Hi Stephen,
>
> I confirmed that issuing parseStrict() was irrelevant. The incorrect
> text won't throw the exception in "lenient" mode as well. In addition,
> "builder" is always instantiated in AbstractTestPrinterParser on each
> TestNG invocation and "strict" is the default mode. Thus I did not
> explicitly issue parseStrict() in the test.
>
> BTW, I found typos wrt the default parse mode. Included the corrections
> in this webrev as well:
>
> http://cr.openjdk.java.net/~naoto/8230136/webrev.01/
>
> Naoto
>
> On 9/11/19 2:01 AM, Stephen Colebourne wrote:
> > The bug references parseStrict() but the test does not. Is the builder
> > already set to parseStrict() ? Anyway, if the bug is to be clearly
> > squished, parseStrict() should appear somewhere.
> > Stephen
> >
> > On Tue, 10 Sep 2019 at 23:42, Joe Wang  wrote:
> >>
> >> +1, looks good to me.
> >>
> >> Best regards,
> >> Joe
> >>
> >> On 9/10/19 2:20 PM, naoto.s...@oracle.com wrote:
> >>> Hello,
> >>>
> >>> Please review the fix to the following issue:
> >>>
> >>> https://bugs.openjdk.java.net/browse/JDK-8230136
> >>>
> >>> The proposed changeset is located at:
> >>>
> >>> http://cr.openjdk.java.net/~naoto/8230136/webrev.00/
> >>>
> >>> The fix is to correct the condition of the invalid case, as suggested
> >>> in the bug report.
> >>>
> >>> Naoto
> >>

Re: [14] RFR: 8230136: DateTimeFormatterBuilder.FractionPrinterParser#parse fails to verify minWidth

[Stephen Colebourne]

The bug references parseStrict() but the test does not. Is the builder
already set to parseStrict() ? Anyway, if the bug is to be clearly
squished, parseStrict() should appear somewhere.
Stephen

On Tue, 10 Sep 2019 at 23:42, Joe Wang  wrote:
>
> +1, looks good to me.
>
> Best regards,
> Joe
>
> On 9/10/19 2:20 PM, naoto.s...@oracle.com wrote:
> > Hello,
> >
> > Please review the fix to the following issue:
> >
> > https://bugs.openjdk.java.net/browse/JDK-8230136
> >
> > The proposed changeset is located at:
> >
> > http://cr.openjdk.java.net/~naoto/8230136/webrev.00/
> >
> > The fix is to correct the condition of the invalid case, as suggested
> > in the bug report.
> >
> > Naoto
>

Re: RFR: 8211990: DateTimeException thrown when calculating duration between certain dates

[Stephen Colebourne]

+1

On Mon, 12 Aug 2019 at 21:44,  wrote:
>
> Hello,
>
> Please review the fix to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8211990
>
> The proposed changeset is located at:
>
> https://cr.openjdk.java.net/~naoto/8211990/webrev.00/
>
> The DateTimeException was thrown due to unconditional conversion beyond
> the valid range of the internal LocalDateTime value. If it happens,
> normalize two instants with the offset of "start" instant. The same kind
> of exception is observed with ZonedDateTime.until(), which is also fixed
> in this changeset.
>
> Naoto

Re: [14] RFR: 8212970: TZ database in "vanguard" format support

[Stephen Colebourne]

Thanks for these changes, +1
Stephen

On Tue, 23 Jul 2019 at 23:18,  wrote:
>
> Hi,
>
> Please review the fix to the following enhancement:
>
> https://bugs.openjdk.java.net/browse/JDK-8212970
>
> The proposed changeset is located at:
>
> https://cr.openjdk.java.net/~naoto/8212970/webrev.09/
>
> This change aims to support the "vanguard" IANA time zone data format,
> which uses the negative savings and transition time beyond a day period.
> The change basically translates those negative savings and transition
> times, such as 25:00, into the ones that the current JDK recognizes,
> then produces the data file "tzdb.dat" at the build time. At the run
> time, the data file is read and interpreted as before. This way the
> produced tzdb.dat is compatible with the prior JDK releases so that the
> TZ Updater can also distribute it as a time zone update.
>
> I have also refactored redundant copy of ZoneRules file in the build
> directory, by dynamically importing the file under src. Thus some build
> related files are modified. I am hoping folks on the build-dev can
> review those changes.
>
> Naoto

Re: [13] RFR: 8223773: DateTimeFormatter Fails to throw an Exception on Invalid CLOCK_HOUR_OF_AMPM and HOUR_OF_AMPM

[Stephen Colebourne]

+1

On Wed, 29 May 2019, 22:13 ,  wrote:

> Hi,
>
> Please review the fix for the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8223773
>
> The proposed changeset is located at:
>
> https://cr.openjdk.java.net/~naoto/8223773/webrev.00/
>
> Checking the range of HourOfAmPm with the range of AmPmOfDay is
> apparently incorrect. Fixing it will change the behavior of parsing with
> SMART resolver style to throw a DateTimeParseException. This is a change
> in which an app will start to see an exception with incorrect data, but
> that is what should be in the first place.
>
> Naoto
>

Re: Proposal: JDK-8148917 Enhanced-For Statement Should Allow Streams

[Stephen Colebourne]

On Thu, 14 Mar 2019 at 19:45, Brian Goetz  wrote:
> Why not make `Iterator` implement `IterableOnce`? The default method
> would obviously just return `this`.
>
> Such a default would not conform to the contract, as IO requires that 
> subsequent calls throw.

IterableOnce.wrap(iterator) ?

Not providing some kind of connection between these types will look
pretty silly I think.
Stephen

Re: Proposal: JDK-8148917 Enhanced-For Statement Should Allow Streams

[Stephen Colebourne]

On Thu, 14 Mar 2019 at 14:21, Brian Goetz  wrote:
> There's a reason it took as long as it did for Stuart to come up with
> this proposal; all the options were known for years, they all have
> problems, and the net benefit is still relatively narrow, which means we
> don't have a lot of budget before the cost-benefit becomes negative.  I
> think the option proposed is the least-worst, and people still seem to
> really want to be able to foreach over streams.

The cost-benefit is definitely tight, but I think it is positive. As I
said here [1] there is still a case for control abstractions in Java
even when you have lambdas


A new concern from me is that this change would allow Iterable and
Stream to be used in foreach, but not Iterator. This seems like an
odd/sharp conceptual edge.

Why not make `Iterator` implement `IterableOnce`? The default method
would obviously just return `this`.

It seems to me that if we are willing to countenance foreach over a
one-shot Stream, it is inappropriate to deny foreach to a one-shot
Iterator.

thanks
Stephen

[1] https://mail.openjdk.java.net/pipermail/amber-dev/2019-March/004127.html

Re: Proposal: JDK-8148917 Enhanced-For Statement Should Allow Streams

[Stephen Colebourne]

On Fri, 1 Mar 2019 at 02:46, Stuart Marks  wrote:
> Please review and comment on this proposal to allow Stream instances to be 
> used
> in enhanced-for ("for-each") loops.

This all seems reasonable enough though of course not ideal.

My slight concern is that the terminal operation is hidden and not
immediately visible, which could be surprising. I do note that streams
throw a clear exception if a terminal operation is called a second
time which mitigates this to some degree.

The IterableOnce class-level Javadoc contradicts the method-level
Javadoc. The class-level say "It is recommended that" whereas the
method-level Javadoc mandates.

Stephen



> Abstract
>
> Occasionally it's useful to iterate a Stream using a conventional loop. 
> However,
> the Stream interface doesn't implement Iterable, and therefore streams cannot 
> be
> used with the enhanced-for statement. This is a proposal to remedy that
> situation by introducing a new interface IterableOnce that is a subtype of
> Iterable, and then retrofitting the Stream interface to implement it. Other 
> JDK
> classes will also be retrofitted to implement IterableOnce.
>
> Full Proposal:
>
>  http://cr.openjdk.java.net/~smarks/reviews/8148917/IterableOnce0.html
>
> Bug report:
>
>  https://bugs.openjdk.java.net/browse/JDK-8148917
>
> Webrev:
>
>  http://cr.openjdk.java.net/~smarks/reviews/8148917/webrev.0/
>
> Note, this changeset isn't ready to push yet. In particular, it has no tests
> yet. However, the implementation is so simple that I figured I should include
> it. Comments on the specification wording are also welcome.
>
> Thanks,
>
> s'marks

Re: RFR - CSR JDK-8215490 Remove String::align

[Stephen Colebourne]

Is String::transform going to be removed as well given the removal of
raw strings and its controversial nature?
Stephen

On Wed, 19 Dec 2018 at 19:50, Jim Laskey  wrote:
>
> CSR: https://bugs.openjdk.java.net/browse/JDK-8215490 
> 
>
> Thank you.
>
> Cheers,
>
> — Jim
>
>
> JBS: https://bugs.openjdk.java.net/browse/JDK-8215489 
> 
> webrev: http://cr.openjdk.java.net/~jlaskey/8215489/webrev/index.html
>

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stephen Colebourne]

Thank you for following up - we all know the core-libs team has a busy
workload, and naming topics are always tricky.

I'm personally unconvinced that `transform()` is the best name out there.
While transform is OK for String and maybe Optional, it is poor for List
and Stream. In addition, I'm still not overly convinced that this is an
appropriate stylistic direction for Java to go more generally (though I
fully agree with point #2 on language change below).

However, since the core-libs team is clearly indicating a desire to close
the topic, I have no personal intention of objecting further..
thanks
Stephen


On Tue, 4 Dec 2018 at 18:38, Stuart Marks  wrote:

> Hi everybody,
>
> I've finally caught up with all of this. I see that several people are
> surprised
> by the way this has turned out. As the first-named reviewer on this
> changeset, I
> felt I should try to figure out what happened.
>
> While this API point stands on its own, this is really part of Jim's RSL
> work
> [1] which includes several API additions to String, and which will likely
> have a
> significant effect on how String literals are used in Java code.
>
> Jim started code review [2] and CSR review [3] for this proposal back in
> September. There was a gap of several weeks, followed by a revised
> proposal on
> 12 Nov [4][5] that changed the method's name from "transform" to "map".
>
> There was further discussion over the next couple days; in particular
> there were
> some objections to the method name "map". On 26 Nov Jim pushed the
> changeset
> with the name changed back to "transform".
>
>  From reading just the messages on the mailing list, I can certainly see
> why
> people were surprised. It looked like there was an issue open for
> discussion,
> yet Jim went ahead and pushed the revised change anyway. What happened?
>
> Given that the primary open issue was about naming, and such mailing list
> discussions can go on for an arbitrarily long time, Jim asked Brian
> off-list for
> a decision on this. The results were:
>
> 1) the name for this method on String should be "transform";
>
> 2) adding library methods is a reasonable way for the platform to provide
> this
> capability (as opposed to various language enhancements that might be
> contemplated); and
>
> 3) the intent is to colonize the name "transform" for this operation on
> this and
> other classes, such as List, Optional, etc.  (It may well be the case that
> there
> is no name that is a good fit for both Stream and for everything else,
> given
> Stream's highly stylized API; we can consider the tradeoffs between
> consistency
> and clarity when we get to that particular one.)
>
> The primary thing that went wrong here was that Brian and Jim neglected to
> circle back to the list to record this decision. This was an oversight.
>
> There could and should have been better communication about this. Brian,
> Jim, or
> I should have documented the results of this decision and followed up on
> the
> mailing list. None of us did. Sorry about that.
>
> What else should be done here?
>
> One thing is that we (I) can make a better effort to keep up on emails,
> though
> the volume -- on a wide variety of topics -- is significant.
>
> Another point is that issues that are raised on the mailing list are often
> discussed and resolved off the mailing list. While we make significant and
> ongoing efforts to write up relevant offline discussions for public
> consumption
> (see, for example, see the recent writeup on nullable values [6]),
> sometimes
> things will fall through the cracks.
>
> Finally, we should also record that this particular decision sets a
> precedent
> for the use of the name "transform" for methods that do similar things on
> other
> classes. To this end, I've updated this bug with a note about "transform":
>
>JDK-8140283 [7] add method to Stream that facilitates fluent chaining
> of
> other methods
>
> and I've also filed the following RFE:
>
>JDK-8214753 [8] (opt) add Optional::transform, allowing an arbitrary
> operation on an Optional
>
> s'marks
>
> [1] http://openjdk.java.net/jeps/326
>
> [2]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055532.html
>
> [3]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055533.html
>
> [4]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056574.html
>
> [5]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html
>
> [6]
>
> http://mail.openjdk.java.net/pipermail/valhalla-spec-experts/2018-November/000784.html
>
> [7] https://bugs.openjdk.java.net/browse/JDK-8140283
>
> [8] https://bugs.openjdk.java.net/browse/JDK-8214753
>
>

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stephen Colebourne]

I see from Twitter (!!!) that this has been pushed. This appears to have
happened without this thread coming to a clear conclusion, particularly wrt
criticism of transform() as a suitable method name in the broader context.

I also do not think that the code review was completed correctly and in
public, which I find concerning.

The two public threads are:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056574.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-November/056592.html

The last webrev had the name map() and no further webrev was published that
I can see. I can see no explicit public approvals of the change from
reviewers. And plenty of concern about the method name, especially map()
but also transform().

While I'm well aware of the danger of public bikeshedding, I also think
that adding methods to `String` deserves more care than this has been
shown. In my view the change should be reverted, and retargetted to Java 13
to allow for a proper conclusion to the discussion.

For info, AWS SDK v2 has chosen applyMutation() for a similar concept:
https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/utils/builder/SdkBuilder.html#applyMutation-java.util.function.Consumer-

Stephen


On Wed, 14 Nov 2018, 14:18 Stephen Colebourne  On Tue, 13 Nov 2018 at 15:44, Brian Goetz  wrote:
> > Yes, we know :)  But we don’t have any current plans to do that, nor
> use-site extension methods, nor does it seem likely to come to the top of
> the language priority list very soon.  So its not a choice between |> and
> .transform(); it’s a choice between transform() and nothing.  Picking
> transform() seems pretty good here.
> >
> > Stephen raised the issue of a “broader context”; indeed, the intention
> of adding a transform() method here was that it would be feasible to add to
> other types for which it was suitable.  String is an obvious candidate for
> “do it first”, but this is within a broader context.
>
> I'd be more comforted if there was a commitment to add the method to
> Stream and Optional in Java 12 or 13.
>
> I also agree with Remi that `transform()` is a poor name for Stream,
> and thus it is a poor name for String. I doubt there is a perfect
> name, process() or apply() seem like good candidates.
>
> Stephen
>

Re: RFR 8177552: Compact Number Formatting support

[Stephen Colebourne]

I'm not a big fan of having a class named `Style` as it is commonly used in
business logic. (Yes, its an inner class, but I still think the potential
for annoyance is high). java.time.* has `TextStyle`, but I don't think it
can be reused here. Maybe the class should be honest and called
NumberFormatStyle (as a top level class).

More generally, the API does not allow the caller to take control of the
text, for example to use "mil" as a suffix for million. eg Think of this
method in java.time.* -
https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/format/DateTimeFormatterBuilder.html#appendText(java.time.temporal.TemporalField,java.util.Map)


Stephen


On Fri, 16 Nov 2018 at 17:56, Nishit Jain  wrote:

> Hi,
>
> Please review this non trivial feature addition to NumberFormat API.
>
> The existing NumberFormat API provides locale based support for
> formatting and parsing numbers which includes formatting decimal,
> percent, currency etc, but the support for formatting a number into a
> human readable or compact form is missing. This RFE adds that feature to
> format a decimal number in a compact format (e.g. 1000 -> 1K, 100 ->
> 1M in en_US locale) , which is useful for the environment where display
> space is limited, so that the formatted string can be displayed in that
> limited space. It is defined by LDML's specification for Compact Number
> Formats.
>
> http://unicode.org/reports/tr35/tr35-numbers.html#Compact_Number_Formats
>
>
> RFE: https://bugs.openjdk.java.net/browse/JDK-8177552
> Webrev: http://cr.openjdk.java.net/~nishjain/8177552/webrevs/webrev.00/
> CSR: https://bugs.openjdk.java.net/browse/JDK-8188147
>
> Request to please help review the the change.
>
> Regards,
> Nishit Jain
>
>
>
>
>

Re: RFR - JDK-8203442 String::transform (Code Review)

[Stephen Colebourne]

On Tue, 13 Nov 2018 at 15:44, Brian Goetz  wrote:
> Yes, we know :)  But we don’t have any current plans to do that, nor use-site 
> extension methods, nor does it seem likely to come to the top of the language 
> priority list very soon.  So its not a choice between |> and .transform(); 
> it’s a choice between transform() and nothing.  Picking transform() seems 
> pretty good here.
>
> Stephen raised the issue of a “broader context”; indeed, the intention of 
> adding a transform() method here was that it would be feasible to add to 
> other types for which it was suitable.  String is an obvious candidate for 
> “do it first”, but this is within a broader context.

I'd be more comforted if there was a commitment to add the method to
Stream and Optional in Java 12 or 13.

I also agree with Remi that `transform()` is a poor name for Stream,
and thus it is a poor name for String. I doubt there is a perfect
name, process() or apply() seem like good candidates.

Stephen

Re: RFR - JDK-8203442 String::transform (Code Review) (was: RFR - JDK-8203442 String::transform (Code Review))

[Stephen Colebourne]

I'm very uncomfortable about adding this method as is.

Firstly, as Peter says (in a different thread), naming this method
`map()` is inconsistent with `Stream` and `Optional`. Adding `map()`
to `String` would be:
  public String map(Function);
Not that such a method would be particularly useful!

Secondly, the premise of the method is effectively an alternative to
the language feature "extension methods". While the motivation in
https://bugs.openjdk.java.net/browse/JDK-8203703 and
https://dzone.com/articles/making-java-fluent-api-more-flexible makes
some sense, it is clearly equally applicable to all types - `String`
is not special here. An isolated change here just looks odd and ill
thought through - a big mistake for the core `String` class.

My preference is for the change to not proceed without consideration
of the broader context. The change should not be added without a
commitment to also add the same method to Stream and Optional as a
minimum, something that would make the addition part of a more
consistent broader design. Doing this would also mandate a different
method name.

FWIW, were such a method added to java.time.* as the OP suggested, it
would enable pluggable conversions:

  java.util.Date ju = LocalDate.now()
.plusDays(2)
.apply(MyUtility::convertToOldDate);

Clearly there is some value to the above in terms of abstraction.
However, there would also be problems making the best choice of types.
Does it go on the concrete LocalDate or the interface Temporal. Its a
problem just like String vs CharSequence.

TLDR, this change needs to be part of a broader context to be
acceptable (something that would inform the best name). Without that
broader context, I'm against this change.

Stephen


On Mon, 12 Nov 2018 at 14:05, Jim Laskey  wrote:
>
> updated webrev: 
> http://cr.openjdk.java.net/~jlaskey/8203442/webrev-02/index.html 
> 
> > On Sep 21, 2018, at 7:42 AM, Remi Forax  wrote:
> >
> > - Mail original -
> >> De: "Alan Bateman" 
> >> À: "Jim Laskey" , "core-libs-dev" 
> >> 
> >> Envoyé: Vendredi 21 Septembre 2018 12:22:42
> >> Objet: Re: RFR - JDK-8203442 String::transform (Code Review)
> >
> >> On 18/09/2018 18:52, Jim Laskey wrote:
> >>> Please review the code for String::transform. The goal is to provide a 
> >>> String
> >>> instance method to allow function application of custom transformations 
> >>> applied
> >>> to an instance of String.
> >>>
> >>> webrev: http://cr.openjdk.java.net/~jlaskey/8203442/webrev/index.html
> >>> jbs: https://bugs.openjdk.java.net/browse/JDK-8203442
> >>> csr: https://bugs.openjdk.java.net/browse/JDK-8203703
> >> I hate to bring up naming but if I have a Stream or
> >> Optional then I'll use the "map" method to apply the mapping
> >> function. Has this already been through the naming bike shed and it
> >> settled on "transform"? Maybe a different name was chosen after looking
> >> at code that would use it in stream operations? Maybe "transform" was
> >> used as the argument to the function is always text? I'm not interested
> >> in re-opening any discussions, just trying to see if options are
> >> captured in a mail somewhere.
> >>
> >> I'm also wondering if this is general enough to be defined by
> >> CharSequence. Retrofitting CharSequence is always problematic and never
> >> done lightly but I'm just wondering if it has been discussed and
> >> dismissed. I don't think it could be done at a later time, at least not
> >> without changing the input type parameter so that the mapping function
> >> is Function rather than Function.
> >
> > the main issue is that a lot of utility methods take a String as parameter, 
> > not a CharSequence :(
> > and Function is a supertype not a subtype of Function > super CharSequence, R>.
> >
> >>
> >> -Alan
> >
> > Rémi
>

Re: Time-zone database issues

[Stephen Colebourne]

On Fri, 26 Oct 2018 at 08:52, Ramanand Patil  wrote:
> Hi Naoto,
> Thank you for filing the bug. As far as tzdata2018f release is concerned I am 
> going ahead with the integration, with the help of the patch provided here- 
> https://mm.icann.org/pipermail/tz/2018-October/027032.html Which avoids 25:00 
> in rearguard format.
> [ https://bugs.openjdk.java.net/browse/JDK-8213016 ]

Yes the right course of action for now is to use the patch that avoids 25:00.

Thanks Naoto for raising JDK-8212970 to cover the longer term change.

Stephen